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

[Matthieu Monrocq]

Hello,

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.

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.

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 ?

-- Matthieu
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110504/4e03af11/attachment.html>