[PATCH] D56405: Split -Wdelete-non-virtual-dtor into two groups

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Jan 8 06:32:32 PST 2019


aaron.ballman added inline comments.


================
Comment at: include/clang/Basic/DiagnosticGroups.td:108-109
 def DeleteNonVirtualDtor : DiagGroup<"delete-non-virtual-dtor">;
+def DeleteAbstractNonVirtualDtor : DiagGroup<"delete-abstract-non-virtual-dtor",
+                                             [DeleteNonVirtualDtor]>;
 def AbstractFinalClass : DiagGroup<"abstract-final-class">;
----------------
rsmith wrote:
> aaron.ballman wrote:
> > rsmith wrote:
> > > This is backwards: this says that `-Wdelete-abstract-non-virtual-dtor` also controls `-Wdelete-non-virtual-dtor`. You presumably want the opposite relationship, so that `-Wdelete-non-virtual-dtor` controls both warnings and `-Wdelete-abstract-non-virtual-dtor` only controls the "abstract" warning.
> > I took this to be the correct order because disabling the abstract case is more dangerous than disabling the non-abstract case (if you disable the abstract one, you're saying "I don't care how bad it gets, don't tell me about it.").
> That seems reasonable as a strategy, but the end result doesn't seem to make much sense: `-Wdelete-abstract-non-virtual-dtor` enables, and  `-Wno-delete-abstract-non-virtual-dtor` disables, warnings that have nothing to do with deleting an abstract class with a non-virtual destructor, and `-Wno-delete-non-virtual-dtor` fails to silence warnings about deleting an object of a class type with a non-virtual destructor. It's also backwards-incompatible, because the meaning of the existing `-W` flag has been changed.
> 
> One way to fix this would be to rename the groups:
> 
> * `delete-abstract-non-virtual-dtor` -> `delete-non-virtual-dtor`
> * `delete-non-virtual-dtor` -> `delete-nonabstract-non-virtual-dtor` (yuck)
> 
> (Or we could keep the existing `delete-abstract-non-virtual-dtor`, add `delete-nonabstract-non-virtual-dtor`, and make `delete-non-virtual-dtor` be a group that contains those other two groups and has no diagnostics of its own.)
> 
> Instead / as well, we could address the false positives more directly: we could only warn if the class in question *introduces* a virtual function (suggesting that it's intended to be used as a base class), rather than warning if the class merely *has* virtual functions (if it overrides virtual functions and doesn't introduce any, there's a good chance it's a leaf class). `-Wdelete-non-virtual-dtor` was supposed to be the "few/no false positives" version of `-Wnon-virtual-dtor` (which is really really just a stylistic warning), and if we can improve it so that people don't want to turn it off, that'd seem better.
> That seems reasonable as a strategy, but the end result doesn't seem to make much sense: -Wdelete-abstract-non-virtual-dtor enables, and  -Wno-delete-abstract-non-virtual-dtor disables, warnings that have nothing to do with deleting an abstract class with a non-virtual destructor, and -Wno-delete-non-virtual-dtor fails to silence warnings about deleting an object of a class type with a non-virtual destructor. It's also backwards-incompatible, because the meaning of the existing -W flag has been changed.

Ah, those are all good points!

> (Or we could keep the existing delete-abstract-non-virtual-dtor, add delete-nonabstract-non-virtual-dtor, and make delete-non-virtual-dtor be a group that contains those other two groups and has no diagnostics of its own.)

I have a slight preference for this approach; it feels a bit more natural to me. However, do we want to spell it `delete-nonabstract-non-virtual-dtor` or `delete-non-abstract-non-virtual-dtor` or `delete-nonabstract-nonvirtual-dtor`? My preference is for anything but the first spelling. ;-)

> Instead / as well, we could address the false positives more directly: 

Yes, improving the fp rate that way would be a great change to make. That said, I would view it as "as well" rather than "instead" because these two diagnostic scenarios seem reasonably separable.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56405/new/

https://reviews.llvm.org/D56405





More information about the cfe-commits mailing list