[clang] Add -Wsuggest-override

[logan-5]

Summary

This patch adds -Wsuggest-override, which allows for more aggressive enforcement of modern C++ best practices, as well as better compatibility with gcc, which has had its own -Wsuggest-override since version 5.1.

Clang already has -Winconsistent-missing-override, which only warns in the case where there is at least one function already marked override in a class. This warning strengthens that warning by suggesting the override keyword regardless of whether it is already present anywhere.

The text between suggest-override and inconsistent-missing-override is now shared, using TextSubstitution for the entire diagnostic text.

[logan-5]

Ran clang-format over the diff.

[logan-5]

Don't warn for destructors by default. This makes -Wsuggest-[destructor-]override more consistent with the behavior of -Winconsistent-missing-[destructor-]override, as well as gcc.

[dblaikie]

Generally Clang tries to avoid stylistic warnings like this (& they are left to be addressed by tools like clang-tidy), hence why -Winconsistent-missing-override was implemented that way (the presence of at least one "override" being a signal the user intended to use override and missed some, rather than that maybe the user hadn't intended to use it at all (because they were compiling their code with multiple versions, etc)). (but I wouldn't outright rule this out - just mentioning the general principle here, perhaps @rsmith or others have other ideas.

[• Quuxplusone]

In D82728#2133720, @dblaikie wrote:

(the presence of at least one "override" being a signal the user intended to use override and missed some [...]

I'm in favor of -Wsuggest-override, and would definitely use it if it existed. The problem I see with -Wmissing-override, as it's been implemented, is that it uses the wrong granularity for "intent": it looks only across the methods of a single class, rather than across all the classes of a single header, or across a single translation unit, or across my entire codebase. In real life, I always want to look across my entire codebase (excluding system headers). If any class in my project uses override, I want Clang to take that as a clear declaration of intent to use override throughout; I don't want Clang treating class A differently from class B. But of course Clang can't look at my whole codebase simultaneously. So the next best thing is to give the user a simple way to "preload the intent flag": to say "As soon as you start processing any class, please act as if intent has been declared for that class." Adding -Wsuggest-override to my build line seems like a perfect way to implement that "preload" facility.

clang/lib/Sema/SemaDeclCXX.cpp

3075

These linebreaks are super unfortunate. Could they be improved by doing it like this?

auto EmitDiag = [this, MD](unsigned DiagDtor, unsigned DiagFn) {
  unsigned DiagID = isa<CXXDestructorDecl>(MD) ? DiagDtor : DiagFn;
  Diag(MD->getLocation(), DiagID) << MD->getDeclName();
  const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
  Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
};
if (Inconsistent)
  EmitDiag(diag::warn_inconsistent_destructor_marked_not_override_overriding,
           diag::warn_inconsistent_function_marked_not_override_overriding);
else
  EmitDiag(diag::warn_suggest_destructor_marked_not_override_overriding
           diag::warn_suggest_function_marked_not_override_overriding);

6760

Can you s/InconsistentOverrideControl/HasInconsistentOverrideControl/ without causing bad linebreaks?

clang/test/SemaCXX/warn-suggest-destructor-override

7

Surely this doesn't compile?!

[dblaikie]

In D82728#2133951, @Quuxplusone wrote:

In D82728#2133720, @dblaikie wrote:

(the presence of at least one "override" being a signal the user intended to use override and missed some [...]

I'm in favor of -Wsuggest-override, and would definitely use it if it existed.

I've no doubt a non-trivial number of folks would - we'd probably enable it in LLVM itself.

The problem I see with -Wmissing-override, as it's been implemented, is that it uses the wrong granularity for "intent": it looks only across the methods of a single class, rather than across all the classes of a single header, or across a single translation unit, or across my entire codebase. In real life, I always want to look across my entire codebase (excluding system headers). If any class in my project uses override, I want Clang to take that as a clear declaration of intent to use override throughout; I don't want Clang treating class A differently from class B. But of course Clang can't look at my whole codebase simultaneously.

Right - Clang's doing its best (well, debatable - that's what we're debating) with what it's got.

So the next best thing is to give the user a simple way to "preload the intent flag": to say "As soon as you start processing any class, please act as if intent has been declared for that class." Adding -Wsuggest-override to my build line seems like a perfect way to implement that "preload" facility.

The issue is that such a warning then needs to be off by default, because we can't assume the user's intent. And Clang's historically been fairly averse to off-by-default warnings due to low user-penetration (not zero, like I said, I imagine LLVM itself would use such a warning, were it implemented) & so concerns about the cost/benefit tradeoff of the added complexity (source code and runtime) of the feature.

[logan-5]

Addressed some feedback.

[logan-5]

Glad this is generating some discussion. For my $0.02, I would also (obviously) love to be able to enable this warning on all the codebases I work on, and this patch was born out of a discussion on the C++ Slack with another user who had found this warning very useful in GCC and was wondering why Clang didn't have it yet.

In D82728#2134072, @dblaikie wrote:

The issue is that such a warning then needs to be off by default, because we can't assume the user's intent. And Clang's historically been fairly averse to off-by-default warnings due to low user-penetration (not zero, like I said, I imagine LLVM itself would use such a warning, were it implemented) & so concerns about the cost/benefit tradeoff of the added complexity (source code and runtime) of the feature.

I agree -Wsuggest-override should be off by default, yet I suspect its user-penetration will be much higher than other off-by-default warnings, due to numerous instances of people on the Internet asking for this feature, as well as the precedent for it set by GCC. Moreover, since this implementation of this warning lies along the exact same code paths as the already existing -Winconsistent-missing-override, the added complexity from this patch is absolutely minimal.

clang/lib/Sema/SemaDeclCXX.cpp

3075

Agreed. Good idea on the fix--needed one more line break (the first one still hit column 81), but it looks much better now.

clang/test/SemaCXX/warn-suggest-destructor-override

7

Because of void virtual? It does, surprisingly, as it does in the test for warn-inconsistent-missing-destructor-override, where I pilfered this from.

Nevertheless, changed to virtual void for sanity's sake.

[dblaikie]

In D82728#2135142, @logan-5 wrote:

Glad this is generating some discussion. For my $0.02, I would also (obviously) love to be able to enable this warning on all the codebases I work on, and this patch was born out of a discussion on the C++ Slack with another user who had found this warning very useful in GCC and was wondering why Clang didn't have it yet.

In D82728#2134072, @dblaikie wrote:

The issue is that such a warning then needs to be off by default, because we can't assume the user's intent. And Clang's historically been fairly averse to off-by-default warnings due to low user-penetration (not zero, like I said, I imagine LLVM itself would use such a warning, were it implemented) & so concerns about the cost/benefit tradeoff of the added complexity (source code and runtime) of the feature.

I agree -Wsuggest-override should be off by default, yet I suspect its user-penetration will be much higher than other off-by-default warnings, due to numerous instances of people on the Internet asking for this feature, as well as the precedent for it set by GCC.

Ah, I hadn't checked/wasn't mentioned whether GCC has this warning. If GCC already has it, there's usually an easy enough argument to be made for GCC compatibility being worthwhile that overcomes most of the objections unless the GCC warning is particularly problematic in some way that I doubt this is.

Is the implementation you're proposing fairly consistent with GCC's? Run it over any big codebases to check it warns in the same places GCC does?

Moreover, since this implementation of this warning lies along the exact same code paths as the already existing -Winconsistent-missing-override, the added complexity from this patch is absolutely minimal.

[logan-5]

In D82728#2135149, @dblaikie wrote:

Is the implementation you're proposing fairly consistent with GCC's? Run it over any big codebases to check it warns in the same places GCC does?

This patch has the same behavior as -Wsuggest-override in GCC >= 9. In GCC <9, it would suggest adding override to void foo() final, but in GCC >=9, final is enough to suppress the warning. This patch's -Wsuggest-override, as well as Clang's pre-existing -Winconsistent-missing-override, are also silenced by final. (https://godbolt.org/z/hbxLK6)

I built Clang itself with a Clang that had this patch, and with GCC with -Wsuggest-override, and compared the results--they were identical (except for the warning text). (618 warnings, for those interested.)

[logan-5]

clang-formatted the diff.

[dblaikie]

In D82728#2136887, @logan-5 wrote:

In D82728#2135149, @dblaikie wrote:

Is the implementation you're proposing fairly consistent with GCC's? Run it over any big codebases to check it warns in the same places GCC does?

This patch has the same behavior as -Wsuggest-override in GCC >= 9. In GCC <9, it would suggest adding override to void foo() final, but in GCC >=9, final is enough to suppress the warning. This patch's -Wsuggest-override, as well as Clang's pre-existing -Winconsistent-missing-override, are also silenced by final. (https://godbolt.org/z/hbxLK6)

I built Clang itself with a Clang that had this patch, and with GCC with -Wsuggest-override, and compared the results--they were identical (except for the warning text). (618 warnings, for those interested.)

Awesome! Really appreciate the data.

[dblaikie]

I'm generally on board with this, but would like @rsmith 's sign off to be sure.

I think it might be nice to make the -Wno-inconsistent-missing-override -Wsuggest-override situation a bit better (by having it still do the same thing as -Wsuggest-override) but I don't feel /too/ strongly about it.

clang/lib/Sema/SemaDeclCXX.cpp

3064–3076

Generally I'd recommend default ref capture [&] on any lambda that doesn't escape its scope - normal scopes don't need to document which variables you use inside them, and I think the same applies to lambdas (bit more debatable when the lambda is named and called later, even within the same scope - so if you feel strongly about keeping it the way it is, that's OK)

clang/test/SemaCXX/warn-suggest-override

4–5

I'd probably simplify these tests by using struct, so everything's implicitly public, rather than class and having to make things public.

Also probably member function declarations rather than definitions would be simpler?

[logan-5]

In D82728#2137021, @dblaikie wrote:

I think it might be nice to make the -Wno-inconsistent-missing-override -Wsuggest-override situation a bit better (by having it still do the same thing as -Wsuggest-override) but I don't feel /too/ strongly about it.

So, ironing this out would mean the code would need this structure:

if (Inconsistent && IsWarningEnabled(-Winconsistent-missing-override))
    Emit(-Winconsistent-missing-override);
else
    Emit(-Wsuggest-override);

The issue is that I wasn't able to find a way to ask if a warning is enabled and make a control flow decision based on that. If there is an API for doing this, it's hidden well. :) So I fell back to just doing if (Inconsistent) instead, which has this quirk if the inconsistent version of the warning is disabled.

clang/lib/Sema/SemaDeclCXX.cpp

3064–3076

I don't feel strongly at all; I'm fine with [&]. I'll make that change.

clang/test/SemaCXX/warn-suggest-override

4–5

Can do.

[logan-5]

Addressed minor feedback.

[dblaikie]

In D82728#2137061, @logan-5 wrote:

In D82728#2137021, @dblaikie wrote:

I think it might be nice to make the -Wno-inconsistent-missing-override -Wsuggest-override situation a bit better (by having it still do the same thing as -Wsuggest-override) but I don't feel /too/ strongly about it.

So, ironing this out would mean the code would need this structure:

if (Inconsistent && IsWarningEnabled(-Winconsistent-missing-override))
    Emit(-Winconsistent-missing-override);
else
    Emit(-Wsuggest-override);

The issue is that I wasn't able to find a way to ask if a warning is enabled and make a control flow decision based on that. If there is an API for doing this, it's hidden well. :) So I fell back to just doing if (Inconsistent) instead, which has this quirk if the inconsistent version of the warning is disabled.

Oh, yep, there's a way - it's usually used for performance-costly warnings, to not spend the resources computing the warning if it's disabled anyway.

Here's an arbitrary example: https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaDecl.cpp#L7237

[logan-5]

In D82728#2137492, @dblaikie wrote:

Oh, yep, there's a way - it's usually used for performance-costly warnings, to not spend the resources computing the warning if it's disabled anyway.

Wow, thanks--overlooked that in a big way. That'll definitely solve the problem. Fixed up patch on the way.

[logan-5]

Fall back to -Wsuggest-override if -Winconsistent-missing-override is disabled.

[dblaikie]

Generally looks good to me (description/commit message should be updated now that the inconsistent inconsistency is no longer an issue)

clang/test/SemaCXX/warn-suggest-destructor-override

2

Does GCC have suggest-destructor-override as a separate warning too?

[logan-5]

clang/test/SemaCXX/warn-suggest-destructor-override

2

It does not. In fact, there's no way to get GCC to warn about destructors missing override. (Which is somewhat defensible, since override is really great for preventing subtle signature mismatches/typos, but destructors don't really have those problems.) However, Clang already has a destructor-specific flavor of the inconsistent-override warning, so I added -Wsuggest-destructor-override for symmetry with -Winconsistent-missing-[destructor-]override.

[logan-5]

clang/test/SemaCXX/warn-suggest-destructor-override

2

Note that this really is the best of both worlds, since it lets Clang's -Wsuggest-override behave identically to GCC's (not warning on destructors), as well as be consistent with its own already existing override warnings (with a special extra flag to enable warnings for destructors).

[logan-5]

Feels like a dumb question, but I'm not sure if and how those build failures are related to this patch? They seem to involve completely separate parts of the LLVM project (and .c files to boot). Was it that the build was failing for an unrelated reason at the moment the bots happened to build this patch? Or am I misunderstanding something more obvious?

[dblaikie]

In D82728#2142071, @logan-5 wrote:

Feels like a dumb question, but I'm not sure if and how those build failures are related to this patch? They seem to involve completely separate parts of the LLVM project (and .c files to boot). Was it that the build was failing for an unrelated reason at the moment the bots happened to build this patch? Or am I misunderstanding something more obvious?

Yeah, don't think those are related, no. Wouldn't worry about them.

Looks good - thanks for the patch and all the details! Might be worth turning on by default in the LLVM build (after all the cleanup)

[logan-5]

In D82728#2146067, @dblaikie wrote:

Looks good - thanks for the patch and all the details! Might be worth turning on by default in the LLVM build (after all the cleanup)

Thanks a lot! I don't (think I) have commit access yada yada, so I'd really appreciate any help getting this committed.

I've already got some of the cleanup in the works (D83611 and D83616), and was planning on taking care of the rest throughout the coming weekish. I'd be happy to turn this warning on in the LLVM build after that.

[xbolva00]

Is it possible to emit fixit note with "override" ?

[logan-5]

In D82728#2161152, @xbolva00 wrote:

Is it possible to emit fixit note with "override" ?

This is a good idea, though unfortunately (after eyeballing the implementation of modernize-use-override in clang-tidy (UseOverrideCheck.cpp)), it looks non-trivial to figure out where exactly to insert override. There's some significant logic in the clang-tidy check involving re-lexing the relevant tokens, to find the insertion point in the presence of complexity like inline definitions, = 0, = {delete|default}, function try blocks, macros, and the list goes on.

The clang-tidy check has this FIXME comment to address the complexity (UseOverrideCheck.cpp:136):

// FIXME: Instead of re-lexing and looking for specific macros such as
// 'ABSTRACT', properly store the location of 'virtual' and '= 0' in each
// FunctionDecl.

If this was done, the resulting information would presumably simplify the clang-tidy check as well as make it easier to add a fixit to this warning. This sounds like an interesting problem, but also like a sizable refactor, and I won't be upset if someone beats me to it. :)

clang/include/clang/Basic/DiagnosticGroups.td

def CXX98CompatPedantic : DiagGroup<"c++98-compat-pedantic",
                                      CXX98CompatBindToTemporaryCopy,
                                      CXX98CompatExtraSemi,
                                      CXXPre14CompatPedantic,
                                      CXXPre17CompatPedantic,
                                      CXXPre20CompatPedantic]>;
 
 def CXX11Narrowing : DiagGroup<"c++11-narrowing">;
 
-def CXX11WarnOverrideDestructor :
+def CXX11WarnInconsistentOverrideDestructor :
   DiagGroup<"inconsistent-missing-destructor-override">;
-def CXX11WarnOverrideMethod : DiagGroup<"inconsistent-missing-override">;
+def CXX11WarnInconsistentOverrideMethod : 
+  DiagGroup<"inconsistent-missing-override">;
+def CXX11WarnSuggestOverrideDestructor : DiagGroup<"suggest-destructor-override">;
+def CXX11WarnSuggestOverride : DiagGroup<"suggest-override", 
+                                         [CXX11WarnSuggestOverrideDestructor]>;
 
 // Original name of this warning in Clang
 def : DiagGroup<"c++0x-narrowing", [CXX11Narrowing]>;
 
 // Name of this warning in GCC
 def : DiagGroup<"narrowing", [CXX11Narrowing]>;
 
 def CXX11CompatReservedUserDefinedLiteral :

clang/include/clang/Basic/DiagnosticSemaKinds.td

 // C++11 override control
 def override_keyword_only_allowed_on_virtual_member_functions : Error<
   "only virtual member functions can be marked '%0'">;
 def override_keyword_hides_virtual_member_function : Error<
   "non-virtual member function marked '%0' hides virtual member "
   "%select{function|functions}1">;
 def err_function_marked_override_not_overriding : Error<
   "%0 marked 'override' but does not override any member functions">;
-def warn_destructor_marked_not_override_overriding : Warning <
-  "%0 overrides a destructor but is not marked 'override'">,
-  InGroup<CXX11WarnOverrideDestructor>, DefaultIgnore;
-def warn_function_marked_not_override_overriding : Warning <
-  "%0 overrides a member function but is not marked 'override'">,
-  InGroup<CXX11WarnOverrideMethod>;
+def warn_destructor_marked_not_override_overriding : TextSubstitution <
+  "%0 overrides a destructor but is not marked 'override'">;
+def warn_function_marked_not_override_overriding : TextSubstitution <
+  "%0 overrides a member function but is not marked 'override'">;
+def warn_inconsistent_destructor_marked_not_override_overriding : Warning <
+  "%sub{warn_destructor_marked_not_override_overriding}0">,
+  InGroup<CXX11WarnInconsistentOverrideDestructor>, DefaultIgnore;
+def warn_inconsistent_function_marked_not_override_overriding : Warning <
+  "%sub{warn_function_marked_not_override_overriding}0">,
+  InGroup<CXX11WarnInconsistentOverrideMethod>;
+def warn_suggest_destructor_marked_not_override_overriding : Warning <
+  "%sub{warn_destructor_marked_not_override_overriding}0">,
+  InGroup<CXX11WarnSuggestOverrideDestructor>, DefaultIgnore;
+def warn_suggest_function_marked_not_override_overriding : Warning <
+  "%sub{warn_function_marked_not_override_overriding}0">,
+  InGroup<CXX11WarnSuggestOverride>, DefaultIgnore;
 def err_class_marked_final_used_as_base : Error<
   "base %0 is marked '%select{final|sealed}1'">;
 def warn_abstract_final_class : Warning<
   "abstract class is marked '%select{final|sealed}0'">, InGroup<AbstractFinalClass>;
 def warn_final_dtor_non_final_class : Warning<
   "class with destructor marked '%select{final|sealed}0' cannot be inherited from">,
   InGroup<FinalDtorNonFinalClass>;
 def note_final_dtor_non_final_class_silence : Note<

clang/include/clang/Sema/Sema.h

public:
 
   bool CheckPureMethod(CXXMethodDecl *Method, SourceRange InitRange);
 
   /// CheckOverrideControl - Check C++11 override control semantics.
   void CheckOverrideControl(NamedDecl *D);
 
   /// DiagnoseAbsenceOfOverrideControl - Diagnose if 'override' keyword was
   /// not used in the declaration of an overriding method.
-  void DiagnoseAbsenceOfOverrideControl(NamedDecl *D);
+  void DiagnoseAbsenceOfOverrideControl(NamedDecl *D, bool Inconsistent);
 
   /// CheckForFunctionMarkedFinal - Checks whether a virtual member function
   /// overrides a virtual member function marked 'final', according to
   /// C++11 [class.virtual]p4.
   bool CheckIfOverriddenFunctionIsMarkedFinal(const CXXMethodDecl *New,
                                               const CXXMethodDecl *Old);
 
 

clang/lib/Sema/SemaDeclCXX.cpp

void Sema::CheckOverrideControl(NamedDecl *D) {
   //   does not override a member function of a base class, the program is
   //   ill-formed.
   bool HasOverriddenMethods = MD->size_overridden_methods() != 0;
   if (MD->hasAttr<OverrideAttr>() && !HasOverriddenMethods)
     Diag(MD->getLocation(), diag::err_function_marked_override_not_overriding)
       << MD->getDeclName();
 }
 
-void Sema::DiagnoseAbsenceOfOverrideControl(NamedDecl *D) {
+void Sema::DiagnoseAbsenceOfOverrideControl(NamedDecl *D, bool Inconsistent) {
   if (D->isInvalidDecl() || D->hasAttr<OverrideAttr>())
     return;
   CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(D);
   if (!MD || MD->isImplicit() || MD->hasAttr<FinalAttr>())
     return;
 
   SourceLocation Loc = MD->getLocation();
   SourceLocation SpellingLoc = Loc;
   if (getSourceManager().isMacroArgExpansion(Loc))
     SpellingLoc = getSourceManager().getImmediateExpansionRange(Loc).getBegin();
   SpellingLoc = getSourceManager().getSpellingLoc(SpellingLoc);
   if (SpellingLoc.isValid() && getSourceManager().isInSystemHeader(SpellingLoc))
       return;
 
   if (MD->size_overridden_methods() > 0) {
-    unsigned DiagID = isa<CXXDestructorDecl>(MD)
-                          ? diag::warn_destructor_marked_not_override_overriding
-                          : diag::warn_function_marked_not_override_overriding;
+    auto EmitDiag = [this, MD](unsigned DiagID) {
       Diag(MD->getLocation(), DiagID) << MD->getDeclName();
       const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
       Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
+    };
+    if (Inconsistent)
+      EmitDiag(isa<CXXDestructorDecl>(MD)
+                  ? diag::warn_inconsistent_destructor_marked_not_override_overriding
+                  : diag::warn_inconsistent_function_marked_not_override_overriding);
+    else
+      EmitDiag(isa<CXXDestructorDecl>(MD)
+                  ? diag::warn_suggest_destructor_marked_not_override_overriding
+                  : diag::warn_suggest_function_marked_not_override_overriding);
   }
 }
 
 /// CheckIfOverriddenFunctionIsMarkedFinal - Checks whether a virtual member
 /// function overrides a virtual member function marked 'final', according to
 /// C++11 [class.virtual]p4.
 bool Sema::CheckIfOverriddenFunctionIsMarkedFinal(const CXXMethodDecl *New,
                                                   const CXXMethodDecl *Old) {
if (auto *M = dyn_cast<CXXMethodDecl>(D)) {
       if (!isa<CXXDestructorDecl>(M))
         CompleteMemberFunction(M);
     } else if (auto *F = dyn_cast<FriendDecl>(D)) {
       CheckForDefaultedFunction(
           dyn_cast_or_null<FunctionDecl>(F->getFriendDecl()));
     }
   }
 
-  if (HasMethodWithOverrideControl &&
-      HasOverridingMethodWithoutOverrideControl) {
-    // At least one method has the 'override' control declared.
-    // Diagnose all other overridden methods which do not have 'override'
-    // specified on them.
+  if (HasOverridingMethodWithoutOverrideControl) {
+    bool InconsistentOverrideControl = HasMethodWithOverrideControl;
     for (auto *M : Record->methods())
-      DiagnoseAbsenceOfOverrideControl(M);
+      DiagnoseAbsenceOfOverrideControl(M, InconsistentOverrideControl);
   }
 
   // Check the defaulted secondary comparisons after any other member functions.
   for (FunctionDecl *FD : DefaultedSecondaryComparisons) {
     CheckExplicitlyDefaultedFunction(S, FD);
 
     // If this is a member function, we deferred checking it until now.
     if (auto *MD = dyn_cast<CXXMethodDecl>(FD))

clang/test/SemaCXX/warn-suggest-destructor-override

+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -Wsuggest-destructor-override
+
+class A {
+ public:
+  ~A() {}
+  void virtual run() {}
+};
+
+class B : public A {
+ public:
+  ~B() {}
+};
+
+class C {
+ public:
+  virtual void run() {}
+  virtual ~C() {}  // expected-note 2{{overridden virtual function is here}}
+};
+
+class D : public C {
+ public:
+  void run() {}
+  ~D() {}
+  // expected-warning@-1 {{'~D' overrides a destructor but is not marked 'override'}}
+};
+
+class E : public C {
+ public:
+  void run() {}
+  virtual ~E() {}
+  // expected-warning@-1 {{'~E' overrides a destructor but is not marked 'override'}}
+};

clang/test/SemaCXX/warn-suggest-override

+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -Wsuggest-override
+
+class A {
+ public:
+  ~A() {}
+  void run() {}
+};
+
+class B : public A {
+ public:
+  ~B() {}
+  void run() {}
+};
+
+class C {
+ public:
+  virtual void run() {} // expected-note 2{{overridden virtual function is here}}
+  virtual ~C() {}  // expected-note 2{{overridden virtual function is here}}
+};
+
+class D : public C {
+ public:
+  void run() {}
+  // expected-warning@-1 {{'run()' overrides a member function but is not marked 'override'}}
+  ~D() {}
+  // expected-warning@-1 {{'~D' overrides a destructor but is not marked 'override'}}
+};
+
+class E : public C {
+ public:
+  virtual void run() {}
+  // expected-warning@-1 {{'run()' overrides a member function but is not marked 'override'}}
+  virtual ~E() {}
+  // expected-warning@-1 {{'~E' overrides a destructor but is not marked 'override'}}
+};
+
+class F : public C {
+ public:
+  void run() override {}
+  ~F() override {}
+};
+
+class G : public C {
+ public:
+  void run() final {}
+  ~G() final {}
+};