Update plugin to handle new style rules for virtual annotations.

tools/clang/plugins/FindBadConstructsConsumer.cpp

Index: tools/clang/plugins/FindBadConstructsConsumer.cpp
diff --git a/tools/clang/plugins/FindBadConstructsConsumer.cpp b/tools/clang/plugins/FindBadConstructsConsumer.cpp
index a3c6e7f4f14b9316dd432cd5f6ff9bf03e1f7320..41c100fe89c13541f78d7b0e7a4d4c3fa12d1ae5 100644
--- a/tools/clang/plugins/FindBadConstructsConsumer.cpp
+++ b/tools/clang/plugins/FindBadConstructsConsumer.cpp
@@ -5,6 +5,7 @@
 #include "FindBadConstructsConsumer.h"
 
 #include "clang/Frontend/CompilerInstance.h"
+#include "clang/AST/Attr.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/Support/raw_ostream.h"
 
@@ -15,9 +16,15 @@ namespace chrome_checker {
 namespace {
 
 const char kMethodRequiresOverride[] =
-    "[chromium-style] Overriding method must be marked with OVERRIDE.";
-const char kMethodRequiresVirtual[] =
-    "[chromium-style] Overriding method must have \"virtual\" keyword.";
+    "[chromium-style] Overriding method must be marked with override or final.";

[hans, 2014/09/26 01:29:29]

pretty nitty, and we probably haven't done this so far, but I think we should
put single quotes around 'override' and 'final here. That corresponds to how
Clang prints its diagnostics.

[dcheng, 2014/09/26 07:07:13]

Done.

+const char kRedundantVirtualSpecifier[] =
+    "[chromium-style] %0 is redundant; %1 implies %0.";

[hans, 2014/09/26 01:29:29]

as per above, I'd put single quotes around the %0's here. (it seems %1 already
gets single quotes, because we're passing in an Attr)

[dcheng, 2014/09/26 07:07:14]

Done, though I opted to just fix the instance where we pass in a string literal
rather than adding quotes here.

+// http://llvm.org/bugs/show_bug.cgi?id=21051 has been filed to make this a
+// Clang warning.
+const char kBaseMethodVirtualAndFinal[] =
+    "[chromium-style] a virtual method on a base class should not also be "
+    "final; "
+    "method should be devirtualized";

[hans, 2014/09/26 01:29:29]

s/on a base class/in a base class/

I'm not sure it would be obvious to everyone what "should be devirtualized"
means here. I'm also not sure about the "base class" part.

Maybe: "[chromium-style] The virtual method does not override anything and is
final; it should be made non-virtual". (But could the fix sometimes be to make
it non-final?)

[dcheng, 2014/09/26 07:07:13]

Updated with your text and weakened the suggestion a bit. I think it's just
worth prodding people to make something non-virtual (they might think about if
it really still needs to be virtual).

 const char kNoExplicitDtor[] =
     "[chromium-style] Classes that are ref-counted should have explicit "
     "destructors that are declared protected or private.";
@@ -59,22 +66,41 @@ const Type* UnwrapType(const Type* type) {
   return type;
 }
 
+// Unfortunately, there doesn't seem to be a good way to determine the location
+// of the 'virtual' keyword. It's available in Declarator, but that isn't
+// accessible from the AST. So instead, make an educated guess that the first
+// token is probably the virtual keyword. Strictly speaking, this doesn't have
+// to be true, but it probably will be.
+// TODO(dcheng): Add a warning to force virtual to always appear first ;-)
+SourceRange GuessSourceRangeForVirtual(const SourceManager& manager,
+                                       const CXXMethodDecl* method) {

[hans, 2014/09/26 01:29:29]

It might be overkill, but you could reach into the SourceManager and check the
bytes at the source range you're looking at here actually say "virtual", and
bail out if it doesn't.

[dcheng, 2014/09/26 07:07:13]

Heh. Done.

+  return SourceRange(method->getLocStart(),
+                     Lexer::getLocForEndOfToken(
+                         method->getLocStart(), 0, manager, LangOptions()));
+}
+
 }  // namespace
 
 FindBadConstructsConsumer::FindBadConstructsConsumer(CompilerInstance& instance,
                                                      const Options& options)
     : ChromeClassTester(instance), options_(options) {
-  // Register warning/error messages.
+  // Messages for virtual method specifiers.
   diag_method_requires_override_ =
       diagnostic().getCustomDiagID(getErrorLevel(), kMethodRequiresOverride);
-  diag_method_requires_virtual_ =
-      diagnostic().getCustomDiagID(getErrorLevel(), kMethodRequiresVirtual);
+  diag_redundant_virtual_specifier_ = diagnostic().getCustomDiagID(
+      getErrorLevel(), kRedundantVirtualSpecifier);
+  diag_base_method_virtual_and_final_ =
+      diagnostic().getCustomDiagID(getErrorLevel(), kBaseMethodVirtualAndFinal);
+
+  // Messages for destructors.
   diag_no_explicit_dtor_ =
       diagnostic().getCustomDiagID(getErrorLevel(), kNoExplicitDtor);
   diag_public_dtor_ =
       diagnostic().getCustomDiagID(getErrorLevel(), kPublicDtor);
   diag_protected_non_virtual_dtor_ =
       diagnostic().getCustomDiagID(getErrorLevel(), kProtectedNonVirtualDtor);
+
+  // Miscellaneous messages.
   diag_weak_ptr_factory_order_ =
       diagnostic().getCustomDiagID(getErrorLevel(), kWeakPtrFactoryOrder);
   diag_bad_enum_last_value_ =
@@ -103,8 +129,7 @@ void FindBadConstructsConsumer::CheckChromeClass(SourceLocation record_location,
 
   bool warn_on_inline_bodies = !implementation_file;
 
-  // Check that all virtual methods are marked accordingly with both
-  // virtual and OVERRIDE.
+  // Check that all virtual methods are annotated with override or final.
   CheckVirtualMethods(record_location, record, warn_on_inline_bodies);
 
   CheckRefCountedDtors(record_location, record);
@@ -250,35 +275,6 @@ void FindBadConstructsConsumer::CheckCtorDtorWeight(
   }
 }
 
-void FindBadConstructsConsumer::CheckVirtualMethod(const CXXMethodDecl* method,
-                                                   bool warn_on_inline_bodies) {
-  if (!method->isVirtual())
-    return;
-
-  if (!method->isVirtualAsWritten()) {
-    SourceLocation loc = method->getTypeSpecStartLoc();
-    if (isa<CXXDestructorDecl>(method))
-      loc = method->getInnerLocStart();
-    SourceManager& manager = instance().getSourceManager();
-    FullSourceLoc full_loc(loc, manager);
-    SourceLocation spelling_loc = manager.getSpellingLoc(loc);
-    diagnostic().Report(full_loc, diag_method_requires_virtual_)
-        << FixItHint::CreateInsertion(spelling_loc, "virtual ");
-  }
-
-  // Virtual methods should not have inline definitions beyond "{}". This
-  // only matters for header files.
-  if (warn_on_inline_bodies && method->hasBody() && method->hasInlineBody()) {
-    if (CompoundStmt* cs = dyn_cast<CompoundStmt>(method->getBody())) {
-      if (cs->size()) {
-        emitWarning(cs->getLBracLoc(),
-                    "virtual methods with non-empty bodies shouldn't be "
-                    "declared inline.");
-      }
-    }
-  }
-}
-
 bool FindBadConstructsConsumer::InTestingNamespace(const Decl* record) {
   return GetNamespace(record).find("testing") != std::string::npos;
 }
@@ -300,47 +296,16 @@ bool FindBadConstructsConsumer::IsMethodInBannedOrTestingNamespace(
   return false;
 }
 
-void FindBadConstructsConsumer::CheckOverriddenMethod(
-    const CXXMethodDecl* method) {
-  if (!method->size_overridden_methods() || method->getAttr<OverrideAttr>())
-    return;
-
-  if (isa<CXXDestructorDecl>(method) || method->isPure())
-    return;
-
-  if (IsMethodInBannedOrTestingNamespace(method))
-    return;
-
-  SourceManager& manager = instance().getSourceManager();
-  SourceRange type_info_range =
-      method->getTypeSourceInfo()->getTypeLoc().getSourceRange();
-  FullSourceLoc loc(type_info_range.getBegin(), manager);
-
-  // Build the FixIt insertion point after the end of the method definition,
-  // including any const-qualifiers and attributes, and before the opening
-  // of the l-curly-brace (if inline) or the semi-color (if a declaration).
-  SourceLocation spelling_end =
-      manager.getSpellingLoc(type_info_range.getEnd());
-  if (spelling_end.isValid()) {
-    SourceLocation token_end =
-        Lexer::getLocForEndOfToken(spelling_end, 0, manager, LangOptions());
-    diagnostic().Report(token_end, diag_method_requires_override_)
-        << FixItHint::CreateInsertion(token_end, " OVERRIDE");
-  } else {
-    diagnostic().Report(loc, diag_method_requires_override_);
-  }
-}
-
-// Makes sure there is a "virtual" keyword on virtual methods.
-//
-// Gmock objects trigger these for each MOCK_BLAH() macro used. So we have a
-// trick to get around that. If a class has member variables whose types are
-// in the "testing" namespace (which is how gmock works behind the scenes),
-// there's a really high chance we won't care about these errors
+// Checks that virtual methods are correctly annotated, and have no body in a
+// header file.
 void FindBadConstructsConsumer::CheckVirtualMethods(
     SourceLocation record_location,
     CXXRecordDecl* record,
     bool warn_on_inline_bodies) {
+  // Gmock objects trigger these for each MOCK_BLAH() macro used. So we have a
+  // trick to get around that. If a class has member variables whose types are
+  // in the "testing" namespace (which is how gmock works behind the scenes),
+  // there's a really high chance we won't care about these errors
   for (CXXRecordDecl::field_iterator it = record->field_begin();
        it != record->field_end();
        ++it) {
@@ -363,9 +328,97 @@ void FindBadConstructsConsumer::CheckVirtualMethods(
     } else if (isa<CXXDestructorDecl>(*it) &&
                !record->hasUserDeclaredDestructor()) {
       // Ignore non-user-declared destructors.
+    } else if (!it->isVirtual()) {
+      continue;
+    } else {
+      CheckVirtualSpecifiers(*it);
+      if (warn_on_inline_bodies)
+        CheckVirtualBodies(*it);
+    }
+  }
+}
+
+// Makes sure that virtual methods use the most appropriate specifier. If a
+// virtual method overrides a method from a base class, the method must use only
+// the override specifier. Furthermore, if the method is not meant to be further
+// subclasses, only the final specifier may be used.

[hans, 2014/09/26 01:29:29]

"not meant to be further subclasses" sounds weird.

[dcheng, 2014/09/26 07:07:13]

Oops, that's a typo. Fixed.

+void FindBadConstructsConsumer::CheckVirtualSpecifiers(
+    const CXXMethodDecl* method) {
+  bool is_override = method->size_overridden_methods() > 0;
+  bool has_virtual = method->isVirtualAsWritten();
+  OverrideAttr* override_attr = method->getAttr<OverrideAttr>();
+  FinalAttr* final_attr = method->getAttr<FinalAttr>();
+
+  if (method->isPure())
+    return;
+
+  if (IsMethodInBannedOrTestingNamespace(method))
+    return;
+
+  if (isa<CXXDestructorDecl>(method) && !options_.strict_virtual_specifiers)
+    return;
+
+  SourceManager& manager = instance().getSourceManager();
+
+  // Complain if a method is annotated virtual && (override || final).
+  if (has_virtual && (override_attr || final_attr) &&
+      options_.strict_virtual_specifiers) {
+    diagnostic().Report(method->getLocStart(),
+                        diag_redundant_virtual_specifier_)
+        << "virtual"
+        << (override_attr ? static_cast<Attr*>(override_attr) : final_attr)
+        << FixItHint::CreateRemoval(
+               GuessSourceRangeForVirtual(manager, method));
+  }
+
+  // Complain if a method is an override and is not annotated with override or
+  // final.
+  if (is_override && !override_attr && !final_attr) {
+    SourceRange type_info_range =
+        method->getTypeSourceInfo()->getTypeLoc().getSourceRange();
+    FullSourceLoc loc(type_info_range.getBegin(), manager);
+
+    // Build the FixIt insertion point after the end of the method definition,
+    // including any const-qualifiers and attributes, and before the opening
+    // of the l-curly-brace (if inline) or the semi-color (if a declaration).
+    SourceLocation spelling_end =
+        manager.getSpellingLoc(type_info_range.getEnd());
+    if (spelling_end.isValid()) {
+      SourceLocation token_end =
+          Lexer::getLocForEndOfToken(spelling_end, 0, manager, LangOptions());
+      diagnostic().Report(token_end, diag_method_requires_override_)
+          << FixItHint::CreateInsertion(token_end, " override");
     } else {
-      CheckVirtualMethod(*it, warn_on_inline_bodies);
-      CheckOverriddenMethod(*it);
+      diagnostic().Report(loc, diag_method_requires_override_);
+    }
+  }
+
+  if (final_attr && override_attr && options_.strict_virtual_specifiers) {
+    diagnostic().Report(override_attr->getLocation(),
+                        diag_redundant_virtual_specifier_)
+        << override_attr << final_attr
+        << FixItHint::CreateRemoval(override_attr->getRange());
+  }
+
+  if (final_attr && !is_override && options_.strict_virtual_specifiers) {
+    diagnostic().Report(method->getLocStart(),
+                        diag_base_method_virtual_and_final_)
+        << FixItHint::CreateRemoval(GuessSourceRangeForVirtual(manager, method))

[hans, 2014/09/26 01:29:29]

Removing the virtual specifier should probably be gated on has_virtual.

[dcheng, 2014/09/26 07:07:13]

It's not needed, because the following does not compile:
class Base {
 public:
  void F() final;
};
will trigger a warning that a non-virtual method cannot be final.

+        << FixItHint::CreateRemoval(final_attr->getRange());
+  }
+}
+
+void FindBadConstructsConsumer::CheckVirtualBodies(
+    const CXXMethodDecl* method) {
+  // Virtual methods should not have inline definitions beyond "{}". This
+  // only matters for header files.
+  if (method->hasBody() && method->hasInlineBody()) {
+    if (CompoundStmt* cs = dyn_cast<CompoundStmt>(method->getBody())) {
+      if (cs->size()) {
+        emitWarning(cs->getLBracLoc(),
+                    "virtual methods with non-empty bodies shouldn't be "
+                    "declared inline.");
+      }
     }
   }
 }