njames93 updated this revision to Diff 309120.
njames93 added a comment.

Added test cases demonstrating copy and move behaviour.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92480/new/

https://reviews.llvm.org/D92480

Files:
  clang/lib/ASTMatchers/ASTMatchersInternal.cpp
  llvm/include/llvm/ADT/IntrusiveRefCntPtr.h
  llvm/unittests/ADT/IntrusiveRefCntPtrTest.cpp

Index: llvm/unittests/ADT/IntrusiveRefCntPtrTest.cpp
===================================================================
--- llvm/unittests/ADT/IntrusiveRefCntPtrTest.cpp
+++ llvm/unittests/ADT/IntrusiveRefCntPtrTest.cpp
@@ -63,4 +63,54 @@
   EXPECT_TRUE(Retained);
 }
 
+namespace {
+class DestructionCounter : public RefCountedBase<DestructionCounter> {
+public:
+  DestructionCounter() {}
+  DestructionCounter(const DestructionCounter &) = default;
+  DestructionCounter(DestructionCounter &&) = default;
+  DestructionCounter &operator=(const DestructionCounter &) = default;
+  DestructionCounter &operator=(DestructionCounter &&) = default;
+
+  ~DestructionCounter() { ++Count; }
+
+  static unsigned getCount() { return Count; }
+  static void resetCount() { Count = 0; }
+
+private:
+  static unsigned Count;
+};
+unsigned DestructionCounter::Count = 0U;
+} // namespace
+
+TEST(IntrusiveRefCntPtr, CopyMove) {
+  {
+
+    IntrusiveRefCntPtr<DestructionCounter> FirstRef = new DestructionCounter();
+    {
+      IntrusiveRefCntPtr<DestructionCounter> SecondRef = FirstRef;
+      // FirstRef and SecondRef both hold the same object, which has a ref count
+      // of 2.
+
+      IntrusiveRefCntPtr<DestructionCounter> Copy =
+          new DestructionCounter(*FirstRef);
+      // Copy holds an object which is a copy of the object stored in FirstRef,
+      // but its ref count is only 1.
+
+      IntrusiveRefCntPtr<DestructionCounter> Move =
+          new DestructionCounter(std::move(*FirstRef));
+      // This follows the same logic as Copy above so its ref count is 1 as
+      // well.
+    }
+    // Copy and Move should destroy their underlying object in the destructor as
+    // the ref count should drop to 0. SecondRef destructor should drop its ref
+    // count to 1, therefore keeping the underlying object alive.
+    EXPECT_EQ(DestructionCounter::getCount(), 2U);
+    DestructionCounter::resetCount();
+  }
+  // Now that FirstRef destructor has ran, the ref count should drop to 0 and
+  // destroy the underlying object
+  EXPECT_EQ(DestructionCounter::getCount(), 1U);
+}
+
 } // end namespace llvm
Index: llvm/include/llvm/ADT/IntrusiveRefCntPtr.h
===================================================================
--- llvm/include/llvm/ADT/IntrusiveRefCntPtr.h
+++ llvm/include/llvm/ADT/IntrusiveRefCntPtr.h
@@ -70,10 +70,27 @@
 template <class Derived> class RefCountedBase {
   mutable unsigned RefCount = 0;
 
-public:
+protected:
   RefCountedBase() = default;
+  // Copy and move constructors/assignments are no-ops as the RefCount isn't
+  // dictated by the class directly.
   RefCountedBase(const RefCountedBase &) {}
+  RefCountedBase(RefCountedBase &&) {}
+  RefCountedBase &operator=(const RefCountedBase &) { return *this; }
+  RefCountedBase &operator=(RefCountedBase &&) { return *this; }
+
+#ifndef NDEBUG
+  ~RefCountedBase() {
+    assert(RefCount == 0 &&
+           "Destruction occured when there are still references to this.");
+  }
+#else
+  // Default the destructor in release builds, A trivial destructor may enable
+  // better codegen.
+  ~RefCountedBase() = default;
+#endif
 
+public:
   void Retain() const { ++RefCount; }
 
   void Release() const {
@@ -85,10 +102,29 @@
 
 /// A thread-safe version of \c RefCountedBase.
 template <class Derived> class ThreadSafeRefCountedBase {
-  mutable std::atomic<int> RefCount;
+  mutable std::atomic<int> RefCount{0};
 
 protected:
-  ThreadSafeRefCountedBase() : RefCount(0) {}
+  ThreadSafeRefCountedBase() = default;
+  ThreadSafeRefCountedBase(const ThreadSafeRefCountedBase &) {}
+  ThreadSafeRefCountedBase(ThreadSafeRefCountedBase &&) {}
+  ThreadSafeRefCountedBase &operator=(const ThreadSafeRefCountedBase &) {
+    return *this;
+  }
+  ThreadSafeRefCountedBase &operator=(ThreadSafeRefCountedBase &&) {
+    return *this;
+  }
+
+#ifndef NDEBUG
+  ~ThreadSafeRefCountedBase() {
+    assert(RefCount == 0 &&
+           "Destruction occured when there are still references to this.");
+  }
+#else
+  // Default the destructor in release builds, A trivial destructor may enable
+  // better codegen.
+  ~ThreadSafeRefCountedBase() = default;
+#endif
 
 public:
   void Retain() const { RefCount.fetch_add(1, std::memory_order_relaxed); }
Index: clang/lib/ASTMatchers/ASTMatchersInternal.cpp
===================================================================
--- clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -187,9 +187,20 @@
   IntrusiveRefCntPtr<DynMatcherInterface> InnerMatcher;
 };
 
+// Use a custom deleter for the TrueMatcherInstance ManagedStatic. This prevents
+// an assert firing when the refcount is nonzero while running its destructor.
+struct DynMatcherInterfaceDeleter {
+  static void call(void *Ptr) {
+    static_cast<DynMatcherInterface *>(Ptr)->Release();
+  }
+};
+
 } // namespace
 
-static llvm::ManagedStatic<TrueMatcherImpl> TrueMatcherInstance;
+static llvm::ManagedStatic<TrueMatcherImpl,
+                           llvm::object_creator<TrueMatcherImpl>,
+                           DynMatcherInterfaceDeleter>
+    TrueMatcherInstance;
 
 bool ASTMatchFinder::isTraversalIgnoringImplicitNodes() const {
   return getASTContext().getParentMapContext().getTraversalKind() ==
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to