[cfe-commits] New warning: delete-non-virtual-dtor

[Matthieu Monrocq]

Hello,

I had talked about changing the implementation of the non-virtual-dtor
warning on cfe-dev, since it's extremely noisy (and for this reason, I
guess, not wired up in -Wmost, -Wall or even -Wextra). The idea was not to
warn at the class definition, but rather to warn whenever `delete` was
called on a polymorphic class that had no virtual destructor.

I tried my hand at it. It's my first foray into Sema though so it may
warrant a thorough review :)

I found  Sema::ActOnCXXDelete  which seems highly suitable for this check,
and indeed there lies the check on the completeness of the class on which
delete is invoked.

I have avoided to warn on the array form of  delete[]  , as far as I know
arrays should never be treated polymorphically (if only because of the
stride).

I could not get it to warn, however, within `~auto_ptr`. I suppose this is
because warnings are suppressed within system headers, however this is
really unfortunate, because even though this warning is indeed emitted
within a system header (pointing at the delete expression there), it is
caused by an incorrect usage of the facility in the first place.

I think that it should be possible to get around this by supplying another
location, by example, upon detection that the location is within a system
header, switch to another warning and tell the user that she should not
build an `auto_ptr` with her class because the destructor does not seem
correct (and pointing at the point of instantiation, where the note is
normally emitted). I don't know if it has already been done however, and if
it is desirable.

Anyway, here is a first stab at introducing this warning.


Note: `final` is not yet available for a class in CLang, I have put a FIXME
on top of the check advising it to be modified so as not to warn on final
classes (when it'll be available).

Note: is there a reason the accessibility check is performed in a separate
`if` statement ? The lines:

    if (const RecordType *RT = PointeeElem->getAs<RecordType>()) {
      CXXRecordDecl *RD = cast<CXXRecordDecl>(RT->getDecl());

appear twice in  Sema::ActOnCXXDelete  and I could not find any reason as to
why it would be necessary to split the two groups. I would suggest the
accessibility check could be moved up into the first.


Summary:
> new DiagnosticGroup DeleteNonVirtualDtor, wired up in Extra (activated
individually by -Wdelete-non-virtual-dtor)
> new Diagnostic in Sema: warn_delete_non_virtual_dtor
> added small check to  Sema::ActOnCXXDelete
> added tests to SemaCXX/destructor.cpp

--Matthieu
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110507/5159e230/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang_warn_delete_non_virtual_destructor.diff
Type: application/octet-stream
Size: 6031 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110507/5159e230/attachment.obj>