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

[Nathan James via Phabricator via llvm-commits]

njames93 added inline comments.


================
Comment at: clang/lib/ASTMatchers/ASTMatchersInternal.cpp:190-196
+// 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();
+  }
+};
----------------
dblaikie wrote:
> I think probably the right solution is to have TrueMatcherImpl's dtor Release the same way its ctor Retains. Symmetry is nice/helps avoid splitting this quirk into two different places.
That method is a nasty can of worms. If TrueMatcherImpl dtor calls `Release`, that `Release` call would realise the RefCount is now zero. That in turn would then call delete on `TrueMatcherImpl` resulting in a double-free.
I think symmetry is possibly the right way to go though, just in using a custom creator that calls Retain itself, remove that responsibility from TrueMatcherImpl. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92480