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 <j...@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 <kf6...@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 <kf6...@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 > > _______________________________________________ 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
