[PATCH] New warning for methods not marked 'override'

[David Blaikie]

  Fly-by thoughts


  (one other minor thought, though we have no consistent strategy on this: what about suppressing the warning in other test files rather than testing it? (I know there's certainly a lot of incidental testing we do in other test files like this))


================
Comment at: lib/Sema/SemaDeclCXX.cpp:4248
@@ +4247,3 @@
+      // Also avoid issuing this diagnostic if not in C++11 mode.
+      if (getLangOpts().CPlusPlus11 && Record->hasOverrideAnnotation())
+        CheckMissingOverrideSpecifier(*this, *M);
----------------
Presumably we want to issue this in C++14, etc. So perhaps "not C++98"?

& also - if you only need to test "hasOverrideAnnotation" here, why not just make it a free function, rather than having to spend a bit in the CXXRecordDecl's data? (I haven't looked in detail, but I assume that's what you're doing)

================
Comment at: test/Misc/diag-override.cpp:9
@@ +8,3 @@
+  virtual void annotated_override() override;
+  // expected-error at +1 {{does not override any member functions}}
+  virtual void not_overriding_anything() override;
----------------
Presumably we already have test coverage for this, don't we? (override on a non-overriding method) It's best not to duplicate test coverage as it just adds maintenance burden when we make code changes (if we change the diagnostic text, for example)

================
Comment at: test/FixIt/fixit-cxx0x.cpp:36
@@ -35,3 +35,3 @@
     F f1,
-      f2 final,
+      f2 final,  // expected-warning {{but is not marked 'override'}}
       f3 override, // expected-error {{expected ';' at end of declaration}}
----------------
Here are some thoughts (possibly wrong):

1) should we let 'final' imply 'override' (I know it's not necessarily true, 'final' could be placed at the start of an inheritance hierarchy)

2) should we use the presence of 'final' to imply that the user wanted to use explicit 'override' (maybe not, I suppose - someone could be using one feature without the other consistently)


http://llvm-reviews.chandlerc.com/D1275