[PATCH] D16041: Change vfs::FileSystem to be managed with std::shared_ptr

Owen Anderson via cfe-commits cfe-commits at lists.llvm.org
Mon Jan 11 14:24:36 PST 2016

> On Jan 11, 2016, at 2:13 PM, Benjamin Kramer <benny.kra at gmail.com> wrote:
> On Mon, Jan 11, 2016 at 8:08 PM, Owen Anderson <resistor at mac.com <mailto:resistor at mac.com>> wrote:
>>> On Jan 11, 2016, at 8:25 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>> On Sun, Jan 10, 2016 at 11:42 PM, Owen Anderson via cfe-commits <cfe-commits at lists.llvm.org> wrote:
>>> resistor created this revision.
>>> resistor added reviewers: chandlerc, bkramer, klimek.
>>> resistor added a subscriber: cfe-commits.
>>> resistor set the repository for this revision to rL LLVM.
>>> Herald added a subscriber: klimek.
>>> Managing it with IntrusiveRefCntPtr caused the virtual destructor not to be called properly.
>>> Regardless of the broader discussion on this patch, I'm confused by why this ^ would be the case. What is it that IntrusiveRefCntPtr is doing that's causing problems with destruction? (& I'm all for changing this to non-intrusive smart pointers over intrusive ones anyway, but I'd still like to understand the extra motivation here)
>> ThreadSafeRefCountedBase, which classes must inherit from in order to use IntrusiveRefCntPtr, does not handle virtual destructors properly.  For the non-thread safe version, there is RefCountBaseVPTR which solves this problem.  An alternative solution would have been to add a corresponding ThreadSafeRefCountedBaseVPTR, but IMO at that point one might as well use shared_ptr since it’s 90% of the way there.
> I find this surprising. ThreadSafeRefCountedBase<FileSystem> calls
> "delete static_cast<const FileSystem *>(this)". As FileSystem has a
> virtual dtor, polymorphic deletion should just work. Am I missing
> something?

I’m not an expert on this stuff, but I’ll refer you to the difference between RefCountedBase and RefCountedBaseVPTR, and point out that ThreadSafeRefCountedBase is like the former rather than the latter.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160111/4e0a8cb4/attachment-0001.html>

More information about the cfe-commits mailing list