Split -Wdelete-non-virtual-dtor into two groups

[erik.pilkington]

Summary

-Wdelete-non-virtual-dtor controlled two diagnostics: 1) calling a non-virtual dtor from an abstract class, and 2) calling a non-virtual dtor from a polymorphic class. 1) is a lot more severe than 2), since 1) is a guaranteed crash, but 2) is just "code smell". Previously, projects compiled with -Wall -Wno-delete-non-virtual-dtor, which is somewhat reasonable, silently crashed on 1).

rdar://40380564

Thanks for taking a look!

[aaron.ballman]

LGTM!

[rsmith]

include/clang/Basic/DiagnosticGroups.td

108–109 ↗

(On Diff #180584)

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.

test/SemaCXX/non-virtual-dtors.cpp

3–4 ↗

(On Diff #180584)

The test also verifies that the -W flags are wrong :)

[aaron.ballman]

include/clang/Basic/DiagnosticGroups.td

108–109 ↗

(On Diff #180584)

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.").

[rsmith]

include/clang/Basic/DiagnosticGroups.td

108–109 ↗

(On Diff #180584)

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.

[aaron.ballman]

include/clang/Basic/DiagnosticGroups.td

108–109 ↗

(On Diff #180584)

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.

[erik.pilkington]

I reverted my commit in r350639.

include/clang/Basic/DiagnosticGroups.td

108–109 ↗

(On Diff #180584)

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.

[erik.pilkington]

Split -Wdelete-non-virtual-dtor into -Wdelete-non-abstract-non-virtual-dtor and -Wdelete-abstract-non-virtual-dtor.

[aaron.ballman]

LGTM!

clang/include/clang/Basic/DiagnosticGroups.td

 def GNUStringLiteralOperatorTemplate :
   DiagGroup<"gnu-string-literal-operator-template">;
 def UndefinedVarTemplate : DiagGroup<"undefined-var-template">;
 def UndefinedFuncTemplate : DiagGroup<"undefined-func-template">;
 def MissingNoEscape : DiagGroup<"missing-noescape">;
 
 def DeleteIncomplete : DiagGroup<"delete-incomplete">;
 def DeleteNonVirtualDtor : DiagGroup<"delete-non-virtual-dtor">;
+def DeleteAbstractNonVirtualDtor : DiagGroup<"delete-abstract-non-virtual-dtor",
+                                             [DeleteNonVirtualDtor]>;
 def AbstractFinalClass : DiagGroup<"abstract-final-class">;
 
 def CXX11CompatDeprecatedWritableStr :
   DiagGroup<"c++11-compat-deprecated-writable-strings">;
 
 def DeprecatedAttributes : DiagGroup<"deprecated-attributes">;
 def DeprecatedDeclarations : DiagGroup<"deprecated-declarations">;
 def UnavailableDeclarations : DiagGroup<"unavailable-declarations">;

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_delete_non_virtual_dtor : Warning<
   "%select{delete|destructor}0 called on non-final %1 that has "
   "virtual functions but non-virtual destructor">,
   InGroup<DeleteNonVirtualDtor>, DefaultIgnore, ShowInSystemHeader;
 def note_delete_non_virtual : Note<
   "qualify call to silence this warning">;
 def warn_delete_abstract_non_virtual_dtor : Warning<
   "%select{delete|destructor}0 called on %1 that is abstract but has "
-  "non-virtual destructor">, InGroup<DeleteNonVirtualDtor>, ShowInSystemHeader;
+  "non-virtual destructor">, InGroup<DeleteAbstractNonVirtualDtor>, ShowInSystemHeader;
 def warn_overloaded_virtual : Warning<
   "%q0 hides overloaded virtual %select{function|functions}1">,
   InGroup<OverloadedVirtual>, DefaultIgnore;
 def note_hidden_overloaded_virtual_declared_here : Note<
   "hidden overloaded virtual function %q0 declared here"
   "%select{|: different classes%diff{ ($ vs $)|}2,3"
   "|: different number of parameters (%2 vs %3)"
   "|: type mismatch at %ordinal2 parameter%diff{ ($ vs $)|}3,4"

clang/test/SemaCXX/non-virtual-dtors.cpp

+// RUN: %clang_cc1 %s -verify -DDIAG1
+// RUN: %clang_cc1 %s -verify -DDIAG1 -DDIAG2 -Wdelete-non-virtual-dtor
+// RUN: %clang_cc1 %s -verify -DDIAG1         -Wmost -Wno-delete-non-virtual-dtor
+// RUN: %clang_cc1 %s -verify                 -Wmost -Wno-delete-abstract-non-virtual-dtor
+
+#ifndef DIAG1
+#ifndef DIAG2
+// expected-no-diagnostics
+#endif
+#endif
+
+struct S1 {
+  ~S1() {}
+  virtual void abs() = 0;
+};
+
+void f1(S1 *s1) { delete s1; }
+#ifdef DIAG1
+// expected-warning@-2 {{delete called on 'S1' that is abstract but has non-virtual destructor}}
+#endif
+
+struct Base {
+  virtual void abs() = 0;
+};
+struct S2 : Base {
+  ~S2() {}
+  void abs() {}
+};
+void f2(S2 *s2) { delete s2; }
+#ifdef DIAG2
+// expected-warning@-2 {{delete called on non-final 'S2' that has virtual functions but non-virtual destructor}}
+#endif