[opensource-dev] Virtual Destructors

[Ricky]

Thanks Josh! That's good to know, and thanks for the link.  While I've
gotten used to a fair number of languages, C++ has a LOT of territory
filled with dark corners and pits.  So I try to proceed cautiously. :D

Ricky
Cron Stardust

On Fri, Jul 1, 2011 at 4:08 PM, Joshua Bell <josh at lindenlab.com> wrote:
> Destructors of derived classes are automatically virtual if the base class
> destructor is virtual.
> http://www.parashift.com/c++-faq-lite/virtual-functions.html#faq-20.7
> The inheritance chain is:
> LLMouseHandler > LLTool > LLManip > LLManipScale
> ... and both LLMouseHandler and LLTool declare the destructor as virtual.
> It wouldn't hurt to explicitly mark ~LLManip and ~LLManipScale as virtual
> for clarity in the code, but it won't actually change anything.
> On Fri, Jul 1, 2011 at 4:02 PM, Ricky <kf6kjg at gmail.com> wrote:
>>
>> Looks like the destructors might only be called at program
>> termination, so this may not be a big problem anyway.  However it IS
>> inconsistent and weird.  And I have it within reach to clean up if it
>> seems good to do so.
>>
>> Ricky
>> Cron Stardust
>>
>> On Fri, Jul 1, 2011 at 3:25 PM, Ricky <kf6kjg at gmail.com> wrote:
>> > Poking around in the llmanip* files working on VWR-25739, I started to
>> > get annoyed at the coding inconsistencies between those files.  So I
>> > started looking at what it would take to make the 3 subclasses
>> > (translate, scale, and rotate) consistent, when I tripped across the
>> > detail that llmaniptranslate.h has the destructor declared virtual
>> > while llmanipscale.h has it declared plainly, and llmaniprotate.h
>> > doesn't explicitly declare a destructor.
>> >
>> > When I looked up some reasons why a destructor should be virtual it
>> > seems that it should be virtual when the class is going to be used in
>> > a polymorphic way and will have delete called on a pointer to it.  IE:
>> > // MyClass is a ParentClass
>> > ParentClass* p = new MyClass();
>> > destroy p;
>> >
>> > Apparently this is about the only case for declaring the destructor
>> > virtual. (see
>> > http://blogs.msdn.com/b/oldnewthing/archive/2004/05/07/127826.aspx
>> > and especially http://www.erata.net/programming/virtual-destructors/ )
>> >  It also comes with a minor performance hit, but that's outside of
>> > scope.
>> >
>> > It turns out that LLManipScale _is_ being used in such a way in
>> > LLToolComp - as are LLManipScale and LLManipRotate:
>> > lltoolcomp.h line 92: LLManip* mManip;
>> > lltoolcomp.cpp line 194: mManip = new LLManipTranslate(this);
>> > lltoolcomp.cpp line 203: delete mManip;
>> > lltoolcomp.cpp line 321: mManip = new LLManipScale(this);
>> > lltoolcomp.cpp line 330: delete mManip;
>> > lltoolcomp.cpp line 520: mManip = new LLManipRotate(this);
>> > lltoolcomp.cpp line 530: delete mManip;
>> >
>> > So it looks like to me that there might be a memory leak in the scale
>> > and rotate classes, as their destructors might NOT be being called.
>> > Of course, Translate's destructor has only an empty definition, and
>> > Rotate doesn't even have one, but Scale does have a full-on
>> > destructor.  And because it is not virtual, it might not be being
>> > called.
>> >
>> > Looking over the history of the files gives me the following:
>> > The Rotate destructor was last touched by Steven Bennets on 2008-03-11
>> > in rev 341 - when LLLinkedList was culled in favor of another
>> > technique.
>> > The Translate destructor was emptied by James Cook on 2009-12-10 in
>> > rev 4496 - switched to a std::vector
>> > The Scale destructor seems to have never existed in revision history.
>> >
>> > Anyone with more familiarity with C++'s nuances in such cases have any
>> > thoughts/suggestions?
>> >
>> > Ricky
>> > Cron Stardust
>> >
>> _______________________________________________
>> Policies and (un)subscribe information available here:
>> http://wiki.secondlife.com/wiki/OpenSource-Dev
>> Please read the policies before posting to keep unmoderated posting
>> privileges
>
>