erik.pilkington reopened this revision. erik.pilkington marked an inline comment as done. erik.pilkington added a comment. This revision is now accepted and ready to land.
I reverted my commit in r350639. ================ 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">; ---------------- aaron.ballman wrote: > 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. > 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. I think the (vast?) majority of users follow the rule that every polymorphic class has a virtual dtor (or, at the very least, would never `delete` a polymorphic class without one). It seems like they would want the diagnostic even if the class didn't introduce any more virtual functions. So I don't want to weaken the "not a guaranteed crash" part of -Wdelete-non-virtual-dtor with this heuristic. We might be able to put it in the "guaranteed crash" half, but that might lead to people disabling it on bad codebases, and I don't think anyone should ever disable the "guaranteed crash" diagnostic. A third option would be to have three flags controlling this, but it doesn't seem like that's worth the complexity. > (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. ;-) Ya, I think this is probably best. It does have the downside that existing "-Wall -Wno-delete-non-virtual-dtor" builds will still silently accept the crasher, but I guess we can't make that do the right thing and still have sane cli semantics. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56405/new/ https://reviews.llvm.org/D56405 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
