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

[Nathan James via Phabricator via llvm-commits]

njames93 added inline comments.


================
Comment at: llvm/include/llvm/Support/ManagedStatic.h:25
+// that are const with no params.
+template <typename T> struct HasRetainRelease {
+private:
----------------
dblaikie wrote:
> njames93 wrote:
> > dblaikie wrote:
> > > Are there many uses that rely on this? I don't think it's really worth all this infrastructure - compared to having it supported on an as-needed basis, such as directly in TrueMatcherImpl's ctor and dtor.
> > It doesn't work in TrueMatcherImpl ctor/dtor due to the whole double free issue(See [[ https://reviews.llvm.org/D92480#inline-864517 | previous comment ]]). I can go back to just supporting it for TrueMatcherImpl by writing a single custom creator/deleter for that class.
> > 
> > This approach taken was a little overkill but likely a little more foolproof for someone using the library. I'll go ahead with whichever approach you would prefer. 
> Ah, right - thanks for walking me through it again, now I better understand your previous comment - sorry for that erroneous suggestion/confusion.
> 
> Fair points all.
> 
> Given all that, I'm sort of leaning towards the idea that maybe the right solution here is for the `TrueMatcherInstance` bear the cost of the complexity here (if it's the only one) with something like:
> 
> ```
> struct TrueMatcherImplCreator {
>   static void *call() {
>     return new IntrusiveRefCntPtr<TrueMatcherImpl>(new TrueMatcherImpl());  
>   }
> };
> static llvm::ManagedStatic<IntrusiveRefCntPtr<TrueMatcherImpl>, TrueMatcherImplCreator> TrueMatcherInstance;
> ```
> 
> I worry about creating a fairly generic extension point for customizing how elements in ManagedStatic can be constructed and destroyed via specialization rather than via explicit creator/destroyer parameters.
> 
> And while the custom destroyer is a bit simpler mechanically (doesn't involve dynamically allocating an IntrusiveCntPtr, which is unintuitive to say the least) - I think sticking to the "if you ever share ownership of a RefCountedBase object, you must've allocated it with 'new' and be really sharing ownership - no lies" is probably a healthier model for RefCountedBase/IntrusiveRefCntPtr.
Decided to take a step back. This is trying to fix a problem that's only here because we are using a `ManagedStatic`. when its not needed. A function scope static has nearly the same semantics of `ManagedStatic` and as this is only used in one function it seems a much better fit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92480