[PATCH] D82728: [clang] Add -Wsuggest-override

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

In D82728#2133720 <https://reviews.llvm.org/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.



================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:3075
+              : diag::
+                    warn_inconsistent_function_marked_not_override_overriding);
+    else
----------------
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);
```


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:6764
+  if (HasOverridingMethodWithoutOverrideControl) {
+    bool InconsistentOverrideControl = HasMethodWithOverrideControl;
     for (auto *M : Record->methods())
----------------
Can you s/InconsistentOverrideControl/HasInconsistentOverrideControl/ without causing bad linebreaks?


================
Comment at: clang/test/SemaCXX/warn-suggest-destructor-override:6
+  ~A() {}
+  void virtual run() {}
+};
----------------
Surely this doesn't compile?!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728