[cfe-dev] is delete on abstract with non-virtal ever safe?

[Matthieu Monrocq]

On Mon, Jul 23, 2012 at 4:29 PM, Henry Miller <hank at millerfarm.com> wrote:

>
> I'm trying to settle an argument we are having at work.  Right now our
> code doesn't compile with clang because of a code like below.
>
> I understand (and agree with) the warning, however the owner of this
> code argues that only the default destructors are used, and they have
> procedures in place to ensure that no derived class will ever have
> members that need destruction.  So the question is he correct that the
> destructor for derived doesn't need to be called?  If so is there a way
> to shut clang up without disabling the warning (which has already found
> several real problems I've since corrected).  If not can someone point
> out why we need a virtual destructor in this case.
>
>     class base
>     {
>     public:
>       virtual void operator() () const = 0;
>     };
>
>     template<typename T>
>     class derived : public base
>     {
>     public:
>        derived(T& d) : data(d) {}
>        void operator() () const {data.DoSomething();}
>
>     private:
>        T& data; // note that the only data is a reference!
>     };
>
>     class myClass
>     {
>     public:
>        void DoSomething() {}
>     };
>
>     int main(int, char **)
>     {
>         myClass cl;
>         base* foo = new derived<myClass>(cl);
>         delete foo;
>     }
>
> clang++ ex.cpp
> ex.cpp:29:5: warning: delete called on 'base' that is abstract but has
>       non-virtual destructor [-Wdelete-non-virtual-dtor]
>     delete foo;
>
> Note, we are comparing to gcc 4.4.  I'm given to understand that gcc4.7
> also has this warning, but we don't have the ability to upgrade right
> now.
>
> --
>   Henry Miller
>   hank at millerfarm.com
>
>
I am very glad to hear that this warning has already been useful, it's
probably my only direct contribution to Clang :)

Regarding shutting it up... well unfortunately whether the destructors are
trivial or not does not matter as far as the Standard is concerned:

*[expr.delete]* *3/* In the first alternative (delete object), if the
static type of the object to be deleted is different from its dynamic type,
the static type shall be a base class of the dynamic type of the object to
be deleted and the static type shall have a virtual destructor *or the
behavior is undefined*.

So even though in practice it probably won't hurt (for now), it is
forbidden by the Standard and any future optimization (of the compiler) or
refactoring (of the code) could potentially trigger a bug.

=> Do you really want to live with that sword of Damocles above your head ?


So I would argue against the owner here: what is the cost, really, of
declaring the destructor virtual ? (especially since there is already
another virtual method anyway)

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