From a8bc4d6266b33a0622e563dda00856d6cbc59294 Mon Sep 17 00:00:00 2001 From: Gleb Mazovetskiy Date: Thu, 22 Nov 2018 04:18:50 +0000 Subject: [PATCH] SharedPtr improvements 1. Moves all logic from `SharedImpl` to `SharedObj` (previously split between the two). 2. Moves all methods into the header so that the compiler can inline them. 3. Adds `operator=(T*)` to avoid the overhead of a temporary `SharedImpl`. 4. Adds C++ unit tests. 5. Removes useless methods: move constructors, value constructors, and move assignments. --- src/memory/SharedPtr.cpp | 83 +----------- src/memory/SharedPtr.hpp | 267 ++++++++++++++++++--------------------- test/Makefile | 16 +++ test/test_shared_ptr.cpp | 162 ++++++++++++++++++++++++ 4 files changed, 299 insertions(+), 229 deletions(-) create mode 100644 test/Makefile create mode 100644 test/test_shared_ptr.cpp diff --git a/src/memory/SharedPtr.cpp b/src/memory/SharedPtr.cpp index 2530360a5..b6c5f93c0 100644 --- a/src/memory/SharedPtr.cpp +++ b/src/memory/SharedPtr.cpp @@ -30,85 +30,4 @@ namespace Sass { #endif bool SharedObj::taint = false; - - SharedObj::SharedObj() - : detached(false) - #ifdef DEBUG_SHARED_PTR - , dbg(false) - #endif - { - refcounter = 0; - #ifdef DEBUG_SHARED_PTR - if (taint) all.push_back(this); - #endif - }; - - SharedObj::~SharedObj() { - #ifdef DEBUG_SHARED_PTR - if (dbg) std::cerr << "Destruct " << this << "\n"; - if(!all.empty()) { // check needed for MSVC (no clue why?) - all.erase(std::remove(all.begin(), all.end(), this), all.end()); - } - #endif - }; - - void SharedPtr::decRefCount() { - if (node) { - -- node->refcounter; - #ifdef DEBUG_SHARED_PTR - if (node->dbg) std::cerr << "- " << node << " X " << node->refcounter << " (" << this << ") " << "\n"; - #endif - if (node->refcounter == 0) { - #ifdef DEBUG_SHARED_PTR - // AST_Node_Ptr ast = dynamic_cast(node); - if (node->dbg) std::cerr << "DELETE NODE " << node << "\n"; - #endif - if (!node->detached) { - delete(node); - } - } - } - } - - void SharedPtr::incRefCount() { - if (node) { - ++ node->refcounter; - node->detached = false; - #ifdef DEBUG_SHARED_PTR - if (node->dbg) { - std::cerr << "+ " << node << " X " << node->refcounter << " (" << this << ") " << "\n"; - } - #endif - } - } - - SharedPtr::~SharedPtr() { - decRefCount(); - } - - - // the create constructor - SharedPtr::SharedPtr(SharedObj* ptr) - : node(ptr) { - incRefCount(); - } - // copy assignment operator - SharedPtr& SharedPtr::operator=(const SharedPtr& rhs) { - void* cur_ptr = (void*) node; - void* rhs_ptr = (void*) rhs.node; - if (cur_ptr == rhs_ptr) { - return *this; - } - decRefCount(); - node = rhs.node; - incRefCount(); - return *this; - } - - // the copy constructor - SharedPtr::SharedPtr(const SharedPtr& obj) - : node(obj.node) { - incRefCount(); - } - -} \ No newline at end of file +} diff --git a/src/memory/SharedPtr.hpp b/src/memory/SharedPtr.hpp index adbb77528..1c9d5653f 100644 --- a/src/memory/SharedPtr.hpp +++ b/src/memory/SharedPtr.hpp @@ -3,6 +3,8 @@ #include "sass/base.h" +#include +#include #include namespace Sass { @@ -39,177 +41,148 @@ namespace Sass { #endif class SharedObj { - protected: - friend class SharedPtr; - friend class Memory_Manager; + public: + SharedObj() : refcount(0), detached(false) { + #ifdef DEBUG_SHARED_PTR + if (taint) all.push_back(this); + #endif + } + virtual ~SharedObj() { + #ifdef DEBUG_SHARED_PTR + all.clear(); + #endif + } + #ifdef DEBUG_SHARED_PTR - static std::vector all; - std::string file; - size_t line; + static void dumpMemLeaks(); + SharedObj* trace(std::string file, size_t line) { + this->file = file; + this->line = line; + return this; + } + std::string getDbgFile() { return file; } + size_t getDbgLine() { return line; } + void setDbg(bool dbg) { this->dbg = dbg; } + size_t getRefCount() const { return refcount; } #endif - static bool taint; - long refcounter; - // long refcount; + + static void setTaint(bool val) { taint = val; } + + virtual const std::string to_string() const = 0; + protected: + friend class SharedPtr; + friend class Memory_Manager; + size_t refcount; bool detached; + static bool taint; #ifdef DEBUG_SHARED_PTR - bool dbg; - #endif - public: - #ifdef DEBUG_SHARED_PTR - static void dumpMemLeaks(); - SharedObj* trace(std::string file, size_t line) { - this->file = file; - this->line = line; - return this; - } + std::string file; + size_t line; + bool dbg = false; + static std::vector all; #endif - SharedObj(); - #ifdef DEBUG_SHARED_PTR - std::string getDbgFile() { - return file; - } - size_t getDbgLine() { - return line; - } - void setDbg(bool dbg) { - this->dbg = dbg; + }; + + class SharedPtr { + public: + SharedPtr() : node(nullptr) {} + SharedPtr(SharedObj* ptr) : node(ptr) { + incRefCount(); + } + SharedPtr(const SharedPtr& obj) : SharedPtr(obj.node) {} + ~SharedPtr() { + decRefCount(); + } + + SharedPtr& operator=(SharedObj* other_node) { + if (node != other_node) { + decRefCount(); + node = other_node; + incRefCount(); + } else if (node != nullptr) { + node->detached = false; } - #endif - static void setTaint(bool val) { - taint = val; + return *this; } - virtual const std::string to_string() const = 0; + SharedPtr& operator=(const SharedPtr& obj) { + return *this = obj.node; + } - virtual ~SharedObj(); - long getRefCount() { - return refcounter; + // Prevents all SharedPtrs from freeing this node until it is assigned to another SharedPtr. + SharedObj* detach() { + if (node != nullptr) node->detached = true; + return node; } - }; + SharedObj* obj() const { return node; } + SharedObj* operator->() const { return node; } + bool isNull() const { return node == nullptr; } + operator bool() const { return node != nullptr; } - class SharedPtr { - protected: + protected: SharedObj* node; - protected: - void decRefCount(); - void incRefCount(); - public: - // the empty constructor - SharedPtr() - : node(NULL) {}; - // the create constructor - SharedPtr(SharedObj* ptr); - // the copy constructor - SharedPtr(const SharedPtr& obj); - // the move constructor - SharedPtr(SharedPtr&& obj); - // copy assignment operator - SharedPtr& operator=(const SharedPtr& obj); - // move assignment operator - SharedPtr& operator=(SharedPtr&& obj); - // pure virtual destructor - virtual ~SharedPtr() = 0; - public: - SharedObj* obj () const { - return node; - }; - SharedObj* operator-> () const { - return node; - }; - bool isNull () { - return node == NULL; - }; - bool isNull () const { - return node == NULL; - }; - SharedObj* detach() const { - if (node) { - node->detached = true; + void decRefCount() { + if (node == nullptr) return; + --node->refcount; + #ifdef DEBUG_SHARED_PTR + if (node->dbg) std::cerr << "- " << node << " X " << node->refcount << " (" << this << ") " << "\n"; + #endif + if (node->refcount == 0 && !node->detached) { + #ifdef DEBUG_SHARED_PTR + if (node->dbg) std::cerr << "DELETE NODE " << node << "\n"; + #endif + delete node; } - return node; - }; - operator bool() const { - return node != NULL; - }; - + } + void incRefCount() { + if (node == nullptr) return; + node->detached = false; + ++node->refcount; + #ifdef DEBUG_SHARED_PTR + if (node->dbg) std::cerr << "+ " << node << " X " << node->refcount << " (" << this << ") " << "\n"; + #endif + } }; - template < class T > + template class SharedImpl : private SharedPtr { - public: - SharedImpl() - : SharedPtr(NULL) {}; - SharedImpl(T* node) - : SharedPtr(node) {}; - template < class U > - SharedImpl(SharedImpl obj) - : SharedPtr(static_cast(obj.ptr())) {} - SharedImpl(T&& node) - : SharedPtr(node) {}; - SharedImpl(const T& node) - : SharedPtr(node) {}; - // the copy constructor - SharedImpl(const SharedImpl& impl) - : SharedPtr(impl.node) {}; - // the move constructor - SharedImpl(SharedImpl&& impl) - : SharedPtr(impl.node) {}; - // copy assignment operator - SharedImpl& operator=(const SharedImpl& rhs) { - if (node) decRefCount(); - node = rhs.node; - incRefCount(); - return *this; + public: + SharedImpl() : SharedPtr(nullptr) {} + + template + SharedImpl(U* node) : + SharedPtr(static_cast(node)) {} + + template + SharedImpl(const SharedImpl& impl) : + SharedImpl(impl.ptr()) {} + + template + SharedImpl& operator=(U *rhs) { + return static_cast&>( + SharedPtr::operator=(static_cast(rhs))); } - // move assignment operator - SharedImpl& operator=(SharedImpl&& rhs) { - // don't move our self - if (this != &rhs) { - if (node) decRefCount(); - node = std::move(rhs.node); - rhs.node = NULL; - } - return *this; + + template + SharedImpl& operator=(const SharedImpl& rhs) { + return static_cast&>( + SharedPtr::operator=(static_cast&>(rhs))); } - // allow implicit conversion to string - // relies on base class implementation operator const std::string() const { if (node) return node->to_string(); - else return std::string("[NULLPTR]"); + return "[nullptr]"; } - ~SharedImpl() {}; - public: - operator T*() const { - return static_cast(this->obj()); - } - operator T&() const { - return *static_cast(this->obj()); - } - T& operator* () const { - return *static_cast(this->obj()); - }; - T* operator-> () const { - return static_cast(this->obj()); - }; - T* ptr () const { - return static_cast(this->obj()); - }; - T* detach() const { - if (this->obj() == NULL) return NULL; - return static_cast(SharedPtr::detach()); - } - bool isNull() const { - return this->obj() == NULL; - } - bool operator<(const T& rhs) const { - return *this->ptr() < rhs; - }; - operator bool() const { - return this->obj() != NULL; - }; + using SharedPtr::isNull; + using SharedPtr::operator bool; + operator T*() const { return static_cast(this->obj()); } + operator T&() const { return *static_cast(this->obj()); } + T& operator* () const { return *static_cast(this->obj()); }; + T* operator-> () const { return static_cast(this->obj()); }; + T* ptr () const { return static_cast(this->obj()); }; + T* detach() { return static_cast(SharedPtr::detach()); } }; } diff --git a/test/Makefile b/test/Makefile new file mode 100644 index 000000000..f76cefce7 --- /dev/null +++ b/test/Makefile @@ -0,0 +1,16 @@ +CXX ?= c++ +CXXFLAGS := -I ../include/ -std=c++11 -fsanitize=address -g -O1 -fno-omit-frame-pointer + +test_shared_ptr: build/test_shared_ptr + @ASAN_OPTIONS="symbolize=1" build/test_shared_ptr + +build: + @mkdir build + +build/test_shared_ptr: | build + @$(CXX) $(CXXFLAGS) -o build/test_shared_ptr test_shared_ptr.cpp ../src/memory/SharedPtr.cpp + +clean: | build + rm -rf build + +.PHONY: test_shared_ptr clean diff --git a/test/test_shared_ptr.cpp b/test/test_shared_ptr.cpp new file mode 100644 index 000000000..6b1ccc2e9 --- /dev/null +++ b/test/test_shared_ptr.cpp @@ -0,0 +1,162 @@ +#include "../src/memory/SharedPtr.hpp" + +#include +#include +#include +#include + +#define ASSERT(cond) \ + if (!(cond)) { \ + std::cerr << "Assertion failed: " #cond " at " __FILE__ << ":" << __LINE__ << std::endl; \ + return false; \ + } \ + +class TestObj : public Sass::SharedObj { + public: + TestObj(bool *destroyed) : destroyed_(destroyed) {} + ~TestObj() { *destroyed_ = true; } + const std::string to_string() const { + std::ostringstream result; + result << "refcount=" << refcount << " destroyed=" << *destroyed_; + return result.str(); + } + private: + bool *destroyed_; +}; + +using SharedTestObj = Sass::SharedImpl; + +bool TestOneSharedPtr() { + bool destroyed = false; + { + SharedTestObj a = new TestObj(&destroyed); + } + ASSERT(destroyed); + return true; +} + +bool TestTwoSharedPtrs() { + bool destroyed = false; + { + SharedTestObj a = new TestObj(&destroyed); + { + SharedTestObj b = a; + } + ASSERT(!destroyed); + } + ASSERT(destroyed); + return true; +} + +bool TestSelfAssignment() { + bool destroyed = false; + { + SharedTestObj a = new TestObj(&destroyed); + a = a; + ASSERT(!destroyed); + } + ASSERT(destroyed); + return true; +} + +bool TestPointerAssignment() { + bool destroyed = false; + std::unique_ptr ptr(new TestObj(&destroyed)); + { + SharedTestObj a = ptr.get(); + } + ASSERT(destroyed); + ptr.release(); + return true; +} + +bool TestOneSharedPtrDetach() { + bool destroyed = false; + std::unique_ptr ptr(new TestObj(&destroyed)); + { + SharedTestObj a = ptr.get(); + a.detach(); + } + ASSERT(!destroyed); + return true; +} + +bool TestTwoSharedPtrsDetach() { + bool destroyed = false; + std::unique_ptr ptr(new TestObj(&destroyed)); + { + SharedTestObj a = ptr.get(); + { + SharedTestObj b = a; + b.detach(); + } + ASSERT(!destroyed); + a.detach(); + } + ASSERT(!destroyed); + return true; +} + +bool TestSelfAssignDetach() { + bool destroyed = false; + std::unique_ptr ptr(new TestObj(&destroyed)); + { + SharedTestObj a = ptr.get(); + a = a.detach(); + ASSERT(!destroyed); + } + ASSERT(destroyed); + ptr.release(); + return true; +} + +bool TestDetachedPtrIsNotDestroyedUntilAssignment() { + bool destroyed = false; + std::unique_ptr ptr(new TestObj(&destroyed)); + { + SharedTestObj a = ptr.get(); + SharedTestObj b = a; + ASSERT(a.detach() == ptr.get()); + ASSERT(!destroyed); + } + ASSERT(!destroyed); + { + SharedTestObj c = ptr.get(); + ASSERT(!destroyed); + } + ASSERT(destroyed); + ptr.release(); + return true; +} + +bool TestDetachNull() { + SharedTestObj a; + ASSERT(a.detach() == nullptr); + return true; +} + +#define TEST(fn) \ + if (fn()) { \ + passed.push_back(#fn); \ + } else { \ + failed.push_back(#fn); \ + std::cerr << "Failed: " #fn << std::endl; \ + } \ + +int main(int argc, char **argv) { + std::vector passed; + std::vector failed; + TEST(TestOneSharedPtr); + TEST(TestTwoSharedPtrs); + TEST(TestSelfAssignment); + TEST(TestPointerAssignment); + TEST(TestOneSharedPtrDetach); + TEST(TestTwoSharedPtrsDetach); + TEST(TestSelfAssignDetach); + TEST(TestDetachedPtrIsNotDestroyedUntilAssignment); + TEST(TestDetachNull); + std::cerr << argv[0] << ": Passed: " << passed.size() + << ", failed: " << failed.size() + << "." << std::endl; + return failed.size(); +}