Now that it will no longer cause warnings (bug 229866), we should eliminate unnecessary virtual destructors. It looks like there are a large number of unneeded virtual destructors throughout the tree. xpidl even generates a virtual destructor in the sample implementation code it outputs. Virtual destructors should only be needed in the rare case where an object may have |delete| called on it through a base class pointer. We should also make destructors private or protected for refcounted objects, since the object will always be deleted from Release().

Note that the virtual destructor is also needed, I believe, for any class that uses NS_IMPL_ISUPPORTS_INHERITED, because that simply forwards to the superclass Release() implementation which then needs to delete the object via the |this| pointer.

Actually, any class that has a subclass using NS_IMPL_ISUPPORTS_INHERITED.

(stashing the patch here while I test on windows and mac)

After wrestling with MSVC, I decided not to change any member functions or data other than the destructor from protected to private. The reason is that MSVC won't allow you to access those members through a |nsRefPtr| due to |nsDerivedSafe|. So, to summarize, this patch makes all of the destructors under xpcom/ non-virtual and private for classes which are: a) never subclassed b) reference counted via nsISupports c) never used on the stack Since making the destructor private makes the class impossible to extend, it's easy to verify that this doesn't break any subclasses which could be depending on a virtual destructor. There are a number of classes that need to have public destructors that could also be made non-virtual, but to make reviewing easier, I'll do that in a separate patch. I also changed the sample code emitted by xpidl to demonstrate making the destructor private and non-virtual. That should be the norm for XPCOM objects. This patch is known to compile on Linux (gcc 3.3), Mac (gcc 3.3), Windows (msvc 6), and HP-UX. It reduces code size of libxpcom by a little over 18KB on Linux.

Comment on attachment 138794 [details] [diff] [review] changes in xpcom/, round 1 >Index: build/dlldeps.cpp >- nsArrayEnumerator(nsnull); Probably safer to change this to new ... >Index: ds/nsSupportsPrimitives.h >Index: glue/nsGenericFactory.h You changed lots of things in these files from private to protected, which should probably be undone.

Comment on attachment 138794 [details] [diff] [review] changes in xpcom/, round 1 >Index: io/nsDirectoryService.cpp >- if (localFile) >- delete localFile; >+ NS_IF_RELEASE(localFile); This should actually be NS_RELEASE. >Index: io/nsStreamUtils.cpp >- virtual ~nsAStreamCopier() >+ ~nsAStreamCopier() needs to be private.

Comment on attachment 138794 [details] [diff] [review] changes in xpcom/, round 1 >Index: obsolete/nsIFileStream.cpp > return NS_ERROR_OUT_OF_MEMORY; > else Get rid of the |else|, and outdent. >Index: reflect/xptinfo/src/xptiprivate.h >+ ~xptiInterfaceInfo() ; No space. >Index: tests/windows/TestCOM.cpp >+private: >+ virtual ~nsTestCom() { Did you mean to remove |virtual|? With those changes (this and prior comments), sr=dbaron.

>Index: io/nsStreamUtils.cpp >- virtual ~nsAStreamCopier() >+ ~nsAStreamCopier() >needs to be private. This is subclassed, so it can't be, and therefore it's not one of the changes I wanted to include in this patch. I'll keep it virtual for now (i.e. no change) > Did you mean to remove |virtual|? yep.

Comment on attachment 138794 [details] [diff] [review] changes in xpcom/, round 1 How did you find these? Did you just grep for "virtual ~" and nail all of the hits that don't have subclasses? Any measurements with this patch? You might want to add a comment in xpidl_header.c as to why the destructor isn't (and shouldn't be if possible) virtual. If this is a real size savings, you may want to post this information onto the newsgroup and writeup a doc for the website. It is a standard C++ things, but looking through LXR this pattern is everywhere.

> How did you find these? Did you just grep for "virtual ~" and nail all of the > hits that don't have subclasses? Yep, starting just under xpcom/. > Any measurements with this patch? I measured the size and found that this cut out about 18 KB (some rodata from vtable size reduction, but mostly code it seems). > You might want to add a comment in xpidl_header.c as to why the destructor > isn't (and shouldn't be if possible) virtual. You mean a comment just in xpidl_header.c, or one that's written into the example implementation? I'd feel kind of silly commenting about why it's not virtual... not virtual is the default. I'd expect to see comments where they _are_ virtual about why. > If this is a real size savings, you may want to post this information onto the > newsgroup and writeup a doc for the website. It is a standard C++ things, but > looking through LXR this pattern is everywhere. I'm not sure it's worthy of a new doc, but certainly any existing docs we have should maybe be updated, and I'll post to the .xpcom newsgroup as well. I'm planning on this taking awhile (and several patches) before we've fixed everything. It's important that people understand that most classes do not need a virtual destructor, even if they are subclassed... it's only needed when a derived class is deleted via a pointer to the base class. And most of our classes cannot be subclassed, in a useful way, from outside of our code, so we should be able to determine exactly when virtual dtors are needed, even if they need to be protected or public.

This patch makes destructors non-virtual on the following classes. I've included a brief explanation of why it's safe to do so. I also commented on several classes that _do_ need virtual destructors about why they're needed. This list includes several classes which are not subclassed, but need to have public destructors so they can be used on the stack (which is why they weren't part of the last patch). nsInt2StrHashtable - not subclassed all subclasses of nsCOMPtr_helper -- these are only ever used on the stack, so no one ever deletes one via a base class pointer PLDHashTableEnumeratorImpl - not subclassed AutoRegEntry - not subclassed nsStaticAtomWrapper - not subclassed AtomImpl - I changed it so that subclass (PermanentAtomImpl) instances are not deleted via a base class pointer nsHashtable - I checked all consumers of the two subclasses (nsObjectHashtable, nsProperties) and they are never deleted via a base class pointer nsSupportsArray - not subclassed nsGenericModule - not subclassed I'm not done with xpcom yet, but I'm breaking this up so it's easier to review. Also, noticably absent from this list is nsVoidArray. It's currently not safe to make that destructor non-virtual but I'm hoping to be able to change the consumers later so that we can do that.

Comment on attachment 139301 [details] [diff] [review] changes in xpcom/, round 2 This patch saves about 10 KB of code size for me (that's not exact, since I'm comparing against a trunk build with some other local changes)

Comment on attachment 139301 [details] [diff] [review] changes in xpcom/, round 2 >Index: base/nsErrorService.cpp This should really be using PRUint32Key. Feel free to fix without coming back for a new sr. >Index: components/nsComponentManager.cpp >+ ~PLDHashTableEnumeratorImpl(); // nonvirtual because this isn't subclassed This should be private. (The one caller calling delete directly should just have already AddRef-ed, and can Release.) >Index: ds/nsHashtable.h >- virtual ~nsHashtable(); >+ ~nsHashtable(); How did you verify that this change is OK? >Index: glue/nsGenericFactory.cpp >@@ -543,12 +543,11 @@ NS_NewGenericModule2(nsModuleInfo* info, > if (!m) > return NS_ERROR_OUT_OF_MEMORY; > >+ NS_ADDREF(m); >+ > // Increase refcnt and store away nsIModule interface to m in return_cobj > rv = m->QueryInterface(NS_GET_IID(nsIModule), (void**)result); >- if (NS_FAILED(rv)) { >- delete m; >- m = nsnull; >- } >+ NS_RELEASE(m); > return rv; > } You really don't need the QueryInterface at all, and it's never going to fail. Just cast to nsIModule* and AddRef.

>>Index: ds/nsHashtable.h >>- virtual ~nsHashtable(); >>+ ~nsHashtable(); > >How did you verify that this change is OK? I lxr'd for users of the two subclasses of nsHashtable (nsObjectHashtable, nsProperties) and made sure that they either used the object as a stack/member variable or deleted it through a derived class pointer.

Comment on attachment 139301 [details] [diff] [review] changes in xpcom/, round 2 checked in

This is pretty much it for XPCOM (there may be a little to do still in nsFileStream.h, but I didn't feel like tracking down a definite answer on whether the dtors need to be virtual). This patch makes a few more destructors virtual... some of them are public as well, and I've commented as to why it's safe to have them be non-virtual. A few other classes were able to join the private virtual destructor club after rearranging the factory function a bit.

Comment on attachment 140651 [details] [diff] [review] changes in xpcom/, round 3 + ~ShortcutResolver(); // nonvirtual since we're not subclassed put the comment above this line to be consistent. I do not understand this change. I think this is for another patch that is in your tree? @@ -1261,7 +1262,8 @@ NS_NewPipe2(nsIAsyncInputStream **pipeIn segmentCount, segmentAlloc); if (NS_FAILED(rv)) { - delete pipe; + NS_ADDREF(pipe); + NS_RELEASE(pipe);

> @@ -1261,7 +1262,8 @@ NS_NewPipe2(nsIAsyncInputStream **pipeIn > segmentCount, > segmentAlloc); > if (NS_FAILED(rv)) { > - delete pipe; > + NS_ADDREF(pipe); > + NS_RELEASE(pipe); This change allowed me to make the dtor for this object private, which is really the easiest way to ensure that it can also be non-virtual.

How did you check that none of the subclasses of nsFileSpec are deleting through a base class pointer?

http://lxr.mozilla.org/seamonkey/source/mailnews/db/msgdb/src/nsMailDatabase.cpp#798 is a potential issue. Probably that member variable could be changed to a derived class pointer or even a derived class object. Once you fix that, sr=dbaron.

Comment on attachment 140651 [details] [diff] [review] changes in xpcom/, round 3 or sr=dbaron if you skip the nsFileSpec change for now

>nsHashtable - I checked all consumers of the two subclasses (nsObjectHashtable, >nsProperties) and they are never deleted via a base class pointer there is another subclass - nsSupportsHashtable, and the ldap code deletes one of these via a base class, so this has introduced leaks into the LDAP code, because it breaks the ref-counting stuff that nsSupportsHashtable does.

I reverted the nsHashtable change. Thanks for the heads-up.

thx, Brian.

Is this still relevant?

Yes.

(In reply to comment #26) > Yes. To be honest, I would rather see the virtual desctructors stay (and have some added) and revert bug 229866. Why? I am using Mozilla code in one of my company's projects and we have g++ warn about classes with virtual functions but non-virtual destructor. The warning is in line with the well-respected Scott Meyers' "Effective C++". BTW: If you remove all the virtual destructors, you have to make sure forever, that they are REALLY not needed, see commend 22 of this ticket, for instance. I don't think its worth the effort. So I would vote for turning the warning back on.