dblaikie added inline comments.
================
Comment at: llvm/include/llvm/Support/ManagedStatic.h:25
+// that are const with no params.
+template <typename T> struct HasRetainRelease {
+private:
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92480/new/
https://reviews.llvm.org/D92480
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
