[PATCH] D92480: [llvm] Add asserts in (ThreadSafe)?RefCountedBase destructors

[Nathan James via Phabricator via cfe-commits]

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

Refactored the ManagedStatic instance into a function static.


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


Index: llvm/include/llvm/ADT/IntrusiveRefCntPtr.h
===================================================================
--- llvm/include/llvm/ADT/IntrusiveRefCntPtr.h
+++ llvm/include/llvm/ADT/IntrusiveRefCntPtr.h
@@ -70,11 +70,23 @@
 template <class Derived> class RefCountedBase {
   mutable unsigned RefCount = 0;
 
-public:
+protected:
   RefCountedBase() = default;
   RefCountedBase(const RefCountedBase &) {}
   RefCountedBase &operator=(const RefCountedBase &) = delete;
 
+#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 {
@@ -94,6 +106,17 @@
   ThreadSafeRefCountedBase &
   operator=(const ThreadSafeRefCountedBase &) = delete;
 
+#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
@@ -150,15 +150,9 @@
 };
 
 /// A matcher that always returns true.
-///
-/// We only ever need one instance of this matcher, so we create a global one
-/// and reuse it to reduce the overhead of the matcher and increase the chance
-/// of cache hits.
 class TrueMatcherImpl : public DynMatcherInterface {
 public:
-  TrueMatcherImpl() {
-    Retain(); // Reference count will never become zero.
-  }
+  TrueMatcherImpl() = default;
 
   bool dynMatches(const DynTypedNode &, ASTMatchFinder *,
                   BoundNodesTreeBuilder *) const override {
@@ -193,8 +187,6 @@
 
 } // namespace
 
-static llvm::ManagedStatic<TrueMatcherImpl> TrueMatcherInstance;
-
 bool ASTMatchFinder::isTraversalIgnoringImplicitNodes() const {
   return getASTContext().getParentMapContext().getTraversalKind() ==
          TK_IgnoreUnlessSpelledInSource;
@@ -273,7 +265,12 @@
 }
 
 DynTypedMatcher DynTypedMatcher::trueMatcher(ASTNodeKind NodeKind) {
-  return DynTypedMatcher(NodeKind, NodeKind, &*TrueMatcherInstance);
+  // We only ever need one instance of TrueMatcherImpl, so we create a static
+  // instance and reuse it to reduce the overhead of the matcher and increase
+  // the chance of cache hits.
+  static const llvm::IntrusiveRefCntPtr<TrueMatcherImpl> Instance =
+      new TrueMatcherImpl();
+  return DynTypedMatcher(NodeKind, NodeKind, Instance);
 }
 
 bool DynTypedMatcher::canMatchNodesOfKind(ASTNodeKind Kind) const {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D92480.309730.patch
Type: text/x-patch
Size: 3021 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20201205/e4411255/attachment.bin>