Open BenPope opened 12 years ago
I'm pretty sure you just end up with different false positives with that approach... but please send an email to cfe-commits if you're looking for more people to comment.
Thank you, I did do some searching but clearly in the wrong places.
Now I see that without dynamic analysis this is less tractable.
The solution is to take note of Herb Sutter (Declare your destructor public and virtual or protected and non-virtual) in which case Derived would have a protected destructor (which is close enough to abstract for our purposes) and take note of Scott Meyers (item 33 of More Effective C++, "Make non-leaf classes abstract."); to ensure the latter with simple static analysis we need to insist that Derived is a leaf class by using final as you suggested.
I wonder, lets think about it another way, can you see any problems with a warning that looks like this:
struct Base { virtual void func() {} protected: ~Base() {} };
struct Derived : Base {};
struct FurtherDerived : Derived {};
clang++ TestSample.cpp -Wderive-public-non-virtual-dtor
TestSample.cpp:9:1: warning: class 'FurtherDerived' is being derived from a class that has virtual functions but public non-virtual destructor [-Wderive-public-non-virtual-dtor] struct FurtherDerived : Derived {}; ^
If you see no major gotchas in that approach, I can take a look at producing a patch (it sounds like fun).
Yes, this issue was brought up when we initially added the warning; see http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20110530/042332.html . You can eliminate the warning in your code by making Derived a final class, as in "struct Derived final : Base {};".
Extended Description
This is also applicable to trunk as of r152547
struct Base { virtual void func() {} protected: ~Base() {} };
struct Derived : Base {};
int main() { Derived * foo = new Derived; delete foo; }
clang++ -Wall file.cpp
If Base::~Base were public, (but nobody deletes through a base) the warning is desirable (with a suggestion to make it protected). If foo is of static type Base and Base::~Base is protected, an error is correctly emitted. If foo is of static type Base* and Base::~Base is public, the warning is correctly emitted.
This appears to be a new warning; in 2.9 it doesn't exist, I suspect -Wnon-virtual-dtor to be related but it wasn't part of -Wall in 2.9.
Motivation: http://www.gotw.ca/publications/mill18.htm Guideline #4
Additional motivation: g++ 4.6.2 gets this right (and I'm led to believe since 4.3) ;)