This is an archive of the discontinued LLVM Phabricator instance.

[ADT] Delete RefCountedBaseVPTR.
ClosedPublic

Authored by jlebar on Dec 29 2016, 10:37 AM.

Details

Summary

This class is unnecessary.

Its comment indicated that it was a compile error to allocate an
instance of a class that inherits from RefCountedBaseVPTR on the stack.
This may have been true at one point, but it's not today.

Moreover you really do not want to allocate *any* refcounted object on
the stack, vptrs or not, so if we did have a way to prevent these
objects from being stack-allocated, we'd want to apply it to regular
RefCountedBase too, obviating the need for a separate RefCountedBaseVPTR
class.

It seems that the main way RefCountedBaseVPTR provides safety is by
making its subclass's destructor virtual. This may have been helpful at
one point, but these days clang will emit an error if you define a class
with virtual functions that inherits from RefCountedBase but doesn't
have a virtual destructor.

Event Timeline

jlebar updated this revision to Diff 82681.Dec 29 2016, 10:37 AM
jlebar retitled this revision from to [ADT] Delete RefCountedBaseVPTR..
jlebar updated this object.
jlebar added reviewers: compnerd, dblaikie.
jlebar added subscribers: llvm-commits, cfe-commits.
dblaikie accepted this revision.Dec 29 2016, 10:45 AM
dblaikie edited edge metadata.

Looks good - thanks for the cleanup!

llvm/unittests/ADT/IntrusiveRefCntPtrTest.cpp
14–37

I'm not sure we even need test coverage for this anymore.

If IntrusiveRefCntPtr calls any dtor (& we should have some coverage of that, to be sure) then it'd be basically impossible for it to call the dtor in a way that wouldn't be compatible with that dtor dispatching if it was virtual. (ie: this test sort of boils down to testing virtual dispatch of virtual dtors - a feature of the compiler, not of this library)

But up to you.

This revision is now accepted and ready to land.Dec 29 2016, 10:45 AM
jlebar marked an inline comment as done.Dec 29 2016, 10:48 AM
jlebar added inline comments.
llvm/unittests/ADT/IntrusiveRefCntPtrTest.cpp
14–37

sgtm. I got rid of the virtual test and moved the explicit checking for destructor calls into the non-virtual test below. Will land the patch with that change.

This revision was automatically updated to reflect the committed changes.
jlebar marked 2 inline comments as done.
llvm/include/llvm/ADT/IntrusiveRefCntPtr.h