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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

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.



================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:3064
   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;
-    Diag(MD->getLocation(), DiagID) << MD->getDeclName();
-    const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
-    Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
+    auto EmitDiag = [this, MD](unsigned DiagDtor, unsigned DiagFn) {
+      unsigned DiagID = isa<CXXDestructorDecl>(MD) ? DiagDtor : DiagFn;
----------------
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)


================
Comment at: clang/test/SemaCXX/warn-suggest-override:3-4
+
+class A {
+ public:
+  ~A() {}
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82728