[cfe-dev] non-virtual-destructor warnings and false positives

[Richard Smith]

On Wed, 4 May 2011 19:03, Matthieu Monrocq wrote:
> I noticed that  -Wnon-virtual-destructor is not activated by default. In
> fact, it is not activated either by  -Wall  or  -Wextra.
>
> This is a bit annoying, because it's an easy error to get into a scenario
>  where one would needed, and the lack of it incurs Undefined Behavior.
>
> However, the way the warning currently is implemented, it reports many
> false positives. The common case I could think of was:
>
> #include <iostream>
> #include <memory>
>
>
> class Base { public:
> virtual int getId() const = 0; protected:
> ~Base() {}
> };
>
>
> class Derived: public Base { // expect-warning {{'Derived' has virtual
> functions but non-virtual destructor [-Wnon-virtual-dtor]}} public:
> explicit Derived(int i): _id(i) {} virtual int getId() const { return _id;
> }
> private:
> int _id; };
>
>
> void print(Base const& b) { std::cout << b.getId() << "\n"; }
>
> int main(int argc, char* argv[]) { {
> Derived d(argc);
> print(d); }
> {
> std::auto_ptr<Derived> b(new Derived(argc));
> print(*b); }
> return 0; }
>
>
> Here, there is absolutely no need for a virtual destructor, yet a warning
> is emitted.
>
> This check is difficult. There are two main issues as I see it:
> - Policy Based design encourages you to inherit (privately, but still)
> from the policies in case they might be empty, to trigger the empty base
> optimization, thus you may inherit from non-polymorphic classes - It is
> impossible to know, at the point of `delete`, whether a class is used
> polymorphically or not (in another TU)
>
> I was wondering if it would be possible to implement this check more
> conservatively, at the point of use: that is, on `delete`. Note that the
> array version does not need to be checked since arrays are not
> polymorphic.

This sounds like a good idea to me. gcc behaves the same way as clang here
(in gcc, the warning is behind -Weffc++), and I've previously wished that
gcc behaves as you describe. I believe the main reason it behaves as it
does is to match 'Effective C++' item 7: "Declare destructors virtual in
polymorphic base classes".

In fact, I think two separate warnings might make sense here: a -Weffc++
warning which behaves as the current warning does, and a more conservative
warning which only warns on 'delete' in the conditions you describe below.

> More specifically, warning for a `delete` called on an object with
> virtual functions but no virtual destructors. Of course, this would still
> be imprecise. In the above case, the destruction of
> `std::auto_ptr<Derived>` is
> perfectly fine, yet would trigger the warning.

In C++0x, we could also suppress the warning for a destructor in a final
class.

> Still it would be less noisy, I think. It would notably nicely interact
> with Inversion of Control technics (such as Dependency Injection) where
> the object is built on the stack and passed by reference to an interface.
>
> Also I believe the check would not be much more expensive that it is now
> (I
> don't think there would be much calls to `delete` on a given class in a
> single TU) and would silence many false positives.
>
> Has it been attempted already ?
>
> Is it doomed to fail ?

No idea :)

Richard