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

[Owen Anderson via cfe-commits]

> 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.

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