dblaikie accepted this revision.
dblaikie added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang/lib/ASTMatchers/ASTMatchersInternal.cpp:155
public:
- TrueMatcherImpl() {
- Retain(); // Reference count will never become zero.
- }
+ TrueMatcherImpl() = default;
----------------
You can remove this ctor declaration, since it's the same as the implicit
default.
================
Comment at: llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:73
-public:
+protected:
RefCountedBase() = default;
----------------
You could probably commit this separately - no need for extra review, but it's
separate/independent of the rest of this change, I think?
================
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:
> > 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.
Yeah, I'm good with that - I guess ManagedStatic has the extra
"deallocate/destroy on llvm_shutdown" and this object doesn't seem
big/significant enough that it'll matter to destroy them on shutdown. No doubt
we've got lots of other function-local statics of similar weight/use that we
don't worry about destroying on llvm_shutdown.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92480/new/
https://reviews.llvm.org/D92480
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
