500866 - Warn about base classes with non-virtual destructors

Status: RESOLVED WORKSFORME

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[Jesse Ruderman]

Does GCC warn when a base class has a non-virtual destructor?  If not, it would be nice to have a static analysis for it.

http://shawnwilsher.com/archives/255 is an example of a Firefox bug caused by this pattern.

Comment 1

[georgi - hopefully not receiving bugspam]

these cppcheck warnings may be somewhat relevant to virtual stuff (single example from *old* log):

[./content/base/src/nsStubMutationObserver.h:61]: (error) Class nsStubMutationObserver which is inherited by class nsNodeIterator does not have a virtual destructor

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

One possible way to do this.

Check for any call to a destructor where:

1. The dtor isn't virtual
2. The class has subclasses
3. The caller isn't a subclass dtor

This isn't 100% correct since it could have been the base class that was instantiated. Or the subclass dtor might be empty (as is the case with nsAutoTArray).

However it's possible that this wouldn't give any false positives in the our current codebase, or that they are few enough to annotate (e.g. annotate nsAutoTArray as an ignorable subclass).

Comment 3

[sneha]

I am a  student from SJCE, Mysore India and would like to take up this bug.

Comment 4

[Jesse Ruderman]

Thanks, Sneha.  I suggest reading https://developer.mozilla.org/en/Dehydra and
asking any questions you have in irc.mozilla.org #static.  I look forward to seeing what you come up with!

Comment 5

[sneha]

Hello,
 This is the script to warn for base classes with non-virtual destructors.

 function process_type(t)
{
  if(t.bases)
  {
   for each (let base in t.bases)
   {
     for (var i in base.type.members)
         
       if (base.type.members[i].name.match("~") && !base.type.members[i].name.isVirtual)
        warning( base.type.members[i].name + "This base class destructor must be made virtual")          
   }       
  }
} 

I would ask to take a look over it and suggest improvements.

Comment 6

[Nathan Froyd [:froydnj]]

The compiler warns about this nowadays, and we have fixed all the relevant bits.