Update plugin to handle new style rules for virtual specifiers.

Implements several new checks in the plugin, gated behind a flag:
- Only one of {virtual,override,final} should be ever used.
- Destructors must also be annotated correctly.
- A virtual final method that doesn't override anything should be
  devirtualized.

The test harness for the Chrome plugin has also been updated to stop
it from littering the source tree with object files, and to make the
golden files easier to update.

BUG=417463

Review URL: https://codereview.chromium.org/597863002

Cr-Commit-Position: refs/heads/master@{#297129}

tools/clang/plugins/FindBadConstructsAction.cpp

@@ -36,6 +36,8 @@ bool FindBadConstructsAction::ParseArgs(const CompilerInstance& instance,
       // TODO(tsepez): Enable this by default once http://crbug.com/356815
       // and http://crbug.com/356816 are fixed.
       options_.check_enum_last_value = true;
+    } else if (args[i] == "strict-virtual-specifiers") {
+      options_.strict_virtual_specifiers = true;
     } else {
       parsed = false;
       llvm::errs() << "Unknown clang plugin argument: " << args[i] << "\n";

tools/clang/plugins/FindBadConstructsConsumer.h

@@ -36,10 +36,10 @@ class FindBadConstructsConsumer : public ChromeClassTester {
                             const Options& options);
 
   // ChromeClassTester overrides:
-  virtual void CheckChromeClass(clang::SourceLocation record_location,
-                                clang::CXXRecordDecl* record);
-  virtual void CheckChromeEnum(clang::SourceLocation enum_location,
-                               clang::EnumDecl* enum_decl);
+  void CheckChromeClass(clang::SourceLocation record_location,
+                        clang::CXXRecordDecl* record) override;
+  void CheckChromeEnum(clang::SourceLocation enum_location,
+                       clang::EnumDecl* enum_decl) override;
 
  private:
   // The type of problematic ref-counting pattern that was encountered.
@@ -48,16 +48,14 @@ class FindBadConstructsConsumer : public ChromeClassTester {
   void CheckCtorDtorWeight(clang::SourceLocation record_location,
                            clang::CXXRecordDecl* record);
 
-  void CheckVirtualMethod(const clang::CXXMethodDecl* method,
-                          bool warn_on_inline_bodies);
-
   bool InTestingNamespace(const clang::Decl* record);
   bool IsMethodInBannedOrTestingNamespace(const clang::CXXMethodDecl* method);
 
-  void CheckOverriddenMethod(const clang::CXXMethodDecl* method);
   void CheckVirtualMethods(clang::SourceLocation record_location,
                            clang::CXXRecordDecl* record,
                            bool warn_on_inline_bodies);
+  void CheckVirtualSpecifiers(const clang::CXXMethodDecl* method);
+  void CheckVirtualBodies(const clang::CXXMethodDecl* method);
 
   void CountType(const clang::Type* type,
                  int* trivial_member,
@@ -85,7 +83,8 @@ class FindBadConstructsConsumer : public ChromeClassTester {
   const Options options_;
 
   unsigned diag_method_requires_override_;
-  unsigned diag_method_requires_virtual_;
+  unsigned diag_redundant_virtual_specifier_;
+  unsigned diag_base_method_virtual_and_final_;
   unsigned diag_no_explicit_dtor_;
   unsigned diag_public_dtor_;
   unsigned diag_protected_non_virtual_dtor_;

tools/clang/plugins/Options.h

@@ -11,11 +11,13 @@ struct Options {
   Options()
       : check_base_classes(false),
         check_weak_ptr_factory_order(false),
-        check_enum_last_value(false) {}
+        check_enum_last_value(false),
+        strict_virtual_specifiers(false) {}
 
   bool check_base_classes;
   bool check_weak_ptr_factory_order;
   bool check_enum_last_value;
+  bool strict_virtual_specifiers;
 };
 
 }  // namespace chrome_checker

tools/clang/plugins/tests/blacklisted_dirs.txt

-/src/chromium/src/myheader.h:2:21: warning: [chromium-style] Overriding method must be marked with OVERRIDE.
+/src/chromium/src/myheader.h:2:21: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
   virtual void foo();  // Should warn about missing 'override'.
                     ^
-                     OVERRIDE
-/src/chrome-breakpad/src/myheader.h:124:21: warning: [chromium-style] Overriding method must be marked with OVERRIDE.
+                     override
+/src/chrome-breakpad/src/myheader.h:124:21: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
   virtual void foo();  // Should warn about missing 'override'.
                     ^
-                     OVERRIDE
+                     override
 2 warnings generated.

tools/clang/plugins/tests/overridden_methods.txt

 In file included from overridden_methods.cpp:5:
-./overridden_methods.h:48:28: warning: [chromium-style] Overriding method must be marked with OVERRIDE.
+./overridden_methods.h:48:28: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
   virtual void SomeMethod();
                            ^
-                            OVERRIDE
-./overridden_methods.h:52:34: warning: [chromium-style] Overriding method must be marked with OVERRIDE.
+                            override
+./overridden_methods.h:52:34: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
   virtual void SomeInlineMethod() {}
                                  ^
-                                  OVERRIDE
-./overridden_methods.h:56:39: warning: [chromium-style] Overriding method must be marked with OVERRIDE.
+                                  override
+./overridden_methods.h:56:39: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
   virtual void SomeConstMethod() const {}
                                       ^
-                                       OVERRIDE
-./overridden_methods.h:58:53: warning: [chromium-style] Overriding method must be marked with OVERRIDE.
+                                       override
+./overridden_methods.h:58:53: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
   virtual void SomeMethodWithExceptionSpec() throw() {}
                                                     ^
-                                                     OVERRIDE
-./overridden_methods.h:61:67: warning: [chromium-style] Overriding method must be marked with OVERRIDE.
+                                                     override
+./overridden_methods.h:61:67: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
   virtual void SomeConstMethodWithExceptionSpec() const throw(int) {}
                                                                   ^
-                                                                   OVERRIDE
-./overridden_methods.h:63:39: warning: [chromium-style] Overriding method must be marked with OVERRIDE.
+                                                                   override
+./overridden_methods.h:63:39: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
   virtual void SomeNonPureBaseMethod() {}
                                       ^
-                                       OVERRIDE
-./overridden_methods.h:65:39: warning: [chromium-style] Overriding method must be marked with OVERRIDE.
+                                       override
+./overridden_methods.h:65:39: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
   virtual void SomeMethodWithComment();  // This is a comment.
                                       ^
-                                       OVERRIDE
-./overridden_methods.h:67:46: warning: [chromium-style] Overriding method must be marked with OVERRIDE.
+                                       override
+./overridden_methods.h:67:46: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
   virtual void SomeMethodWithCommentAndBody() {}  // This is a comment.
                                              ^
-                                              OVERRIDE
-overridden_methods.cpp:24:28: warning: [chromium-style] Overriding method must be marked with OVERRIDE.
+                                              override
+overridden_methods.cpp:24:28: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
   virtual void SomeMethod();
                            ^
-                            OVERRIDE
-overridden_methods.cpp:28:34: warning: [chromium-style] Overriding method must be marked with OVERRIDE.
+                            override
+overridden_methods.cpp:28:34: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
   virtual void SomeInlineMethod() {}
                                  ^
-                                  OVERRIDE
-overridden_methods.cpp:32:39: warning: [chromium-style] Overriding method must be marked with OVERRIDE.
+                                  override
+overridden_methods.cpp:32:39: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
   virtual void SomeConstMethod() const {}
                                       ^
-                                       OVERRIDE
-overridden_methods.cpp:34:53: warning: [chromium-style] Overriding method must be marked with OVERRIDE.
+                                       override
+overridden_methods.cpp:34:53: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
   virtual void SomeMethodWithExceptionSpec() throw() {}
                                                     ^
-                                                     OVERRIDE
-overridden_methods.cpp:37:67: warning: [chromium-style] Overriding method must be marked with OVERRIDE.
+                                                     override
+overridden_methods.cpp:37:67: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
   virtual void SomeConstMethodWithExceptionSpec() const throw(int) {}
                                                                   ^
-                                                                   OVERRIDE
-overridden_methods.cpp:39:39: warning: [chromium-style] Overriding method must be marked with OVERRIDE.
+                                                                   override
+overridden_methods.cpp:39:39: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
   virtual void SomeNonPureBaseMethod() {}
                                       ^
-                                       OVERRIDE
-overridden_methods.cpp:41:39: warning: [chromium-style] Overriding method must be marked with OVERRIDE.
+                                       override
+overridden_methods.cpp:41:39: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
   virtual void SomeMethodWithComment();  // This is a comment.
                                       ^
-                                       OVERRIDE
-overridden_methods.cpp:43:46: warning: [chromium-style] Overriding method must be marked with OVERRIDE.
+                                       override
+overridden_methods.cpp:43:46: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
   virtual void SomeMethodWithCommentAndBody() {}  // This is a comment.
                                              ^
-                                              OVERRIDE
+                                              override
 16 warnings generated.

tools/clang/plugins/tests/test.sh

@@ -33,7 +33,7 @@ do_testcase() {
     flags="${flags} -isysroot $(xcrun --show-sdk-path) -stdlib=libstdc++"
   fi
 
-  local output="$("${CLANG_PATH}" -c -Wno-c++11-extensions \
+  local output="$("${CLANG_PATH}" -fsyntax-only -Wno-c++11-extensions \
       -Xclang -load -Xclang "${PLUGIN_PATH}" \
       -Xclang -add-plugin -Xclang find-bad-constructs ${flags} ${1} 2>&1)"
   local diffout="$(echo "${output}" | diff - "${2}")"
@@ -44,6 +44,10 @@ do_testcase() {
     echo "FAIL: ${1}"
     echo "Output of compiler:"
     echo "${output}"
+    cat > ${2}-actual << EOF
+${output}
+EOF
+
     echo "Expected output:"
     cat "${2}"
     echo

tools/clang/plugins/tests/virtual_base_method_also_final.cpp

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#define VIRTUAL virtual
+#define VIRTUAL_VOID virtual void
+
+class A {
+ public:
+  VIRTUAL void F() final {}
+  // Make sure an out-of-place virtual doesn't cause an incorrect fixit removal
+  // to be emitted.
+  void VIRTUAL G() final {}
+  // Make sure a fixit removal isn't generated for macros that expand to more
+  // than just 'virtual'.
+  VIRTUAL_VOID H() final {}
+};

tools/clang/plugins/tests/virtual_base_method_also_final.flags

+-Xclang -plugin-arg-find-bad-constructs -Xclang strict-virtual-specifiers

tools/clang/plugins/tests/virtual_base_method_also_final.txt

+virtual_base_method_also_final.cpp:10:3: warning: [chromium-style] 'virtual' is redundant; 'final' implies 'virtual'.
+  VIRTUAL void F() final {}
+  ^~~~~~~~
+virtual_base_method_also_final.cpp:5:17: note: expanded from macro 'VIRTUAL'
+#define VIRTUAL virtual
+                ^
+virtual_base_method_also_final.cpp:10:3: warning: [chromium-style] The virtual method does not override anything and is final; consider making it non-virtual.
+  VIRTUAL void F() final {}
+  ^~~~~~~~         ~~~~~~
+virtual_base_method_also_final.cpp:5:17: note: expanded from macro 'VIRTUAL'
+#define VIRTUAL virtual
+                ^
+virtual_base_method_also_final.cpp:13:3: warning: [chromium-style] 'virtual' is redundant; 'final' implies 'virtual'.
+  void VIRTUAL G() final {}
+  ^
+virtual_base_method_also_final.cpp:13:3: warning: [chromium-style] The virtual method does not override anything and is final; consider making it non-virtual.
+  void VIRTUAL G() final {}
+  ^                ~~~~~~
+virtual_base_method_also_final.cpp:16:3: warning: [chromium-style] 'virtual' is redundant; 'final' implies 'virtual'.
+  VIRTUAL_VOID H() final {}
+  ^
+virtual_base_method_also_final.cpp:6:22: note: expanded from macro 'VIRTUAL_VOID'
+#define VIRTUAL_VOID virtual void
+                     ^
+virtual_base_method_also_final.cpp:16:3: warning: [chromium-style] The virtual method does not override anything and is final; consider making it non-virtual.
+virtual_base_method_also_final.cpp:6:22: note: expanded from macro 'VIRTUAL_VOID'
+#define VIRTUAL_VOID virtual void
+                     ^
+6 warnings generated.

tools/clang/plugins/tests/virtual_methods.cpp → tools/clang/plugins/tests/virtual_bodies.cpp

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "virtual_methods.h"
+#include "virtual_bodies.h"
 
 // Shouldn't warn about method usage in the implementation file.
 class VirtualMethodsInImplementation {
@@ -26,9 +26,12 @@ class ConcreteVirtualMethodsInImplementation
 };
 
 // Fill in the implementations
-void VirtualMethodsInHeaders::MethodHasNoArguments() {}
-void WarnOnMissingVirtual::MethodHasNoArguments() {}
-void VirtualMethodsInImplementation::MethodHasNoArguments() {}
+void VirtualMethodsInHeaders::MethodHasNoArguments() {
+}
+void WarnOnMissingVirtual::MethodHasNoArguments() {
+}
+void VirtualMethodsInImplementation::MethodHasNoArguments() {
+}
 
 int main() {
   ConcreteVirtualMethodsInHeaders one;

tools/clang/plugins/tests/virtual_methods.h → tools/clang/plugins/tests/virtual_bodies.h

@@ -32,6 +32,7 @@ class VirtualMethodsInHeadersTesting : public VirtualMethodsInHeaders {
  public:
   // Don't complain about no virtual testing methods.
   void MethodHasNoArguments();
+
  private:
   testing::TestStruct tester_;
 };

tools/clang/plugins/tests/virtual_bodies.txt

+In file included from virtual_bodies.cpp:5:
+./virtual_bodies.h:17:36: warning: [chromium-style] virtual methods with non-empty bodies shouldn't be declared inline.
+  virtual bool ComplainAboutThis() { return true; }
+                                   ^
+1 warning generated.

tools/clang/plugins/tests/virtual_methods.txt

-In file included from virtual_methods.cpp:5:
-./virtual_methods.h:17:36: warning: [chromium-style] virtual methods with non-empty bodies shouldn't be declared inline.
-  virtual bool ComplainAboutThis() { return true; }
-                                   ^
-./virtual_methods.h:23:3: warning: [chromium-style] Overriding method must have "virtual" keyword.
-  void MethodHasNoArguments() override;
-  ^
-  virtual 
-2 warnings generated.

tools/clang/plugins/tests/virtual_specifiers.cpp

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// Tests for chromium style checks for virtual/override/final specifiers on
+// virtual methods.
+
+// Purposely use macros to test that the FixIt hints don't try to remove the
+// macro body.
+#define OVERRIDE override
+#define FINAL final
+
+// Base class can only use virtual.
+class Base {
+ public:
+  virtual ~Base() {}
+  virtual void F() = 0;
+};
+
+// Derived classes correctly use only override or final specifier.
+class CorrectOverride : public Base {
+ public:
+  ~CorrectOverride() OVERRIDE {}
+  void F() OVERRIDE {}
+};
+
+class CorrectFinal : public CorrectOverride {
+ public:
+  ~CorrectFinal() FINAL {}
+  void F() FINAL {}
+};
+
+// No override on an overridden method should trigger a diagnostic.
+class MissingOverride : public Base {
+ public:
+  ~MissingOverride() {}
+  void F() {}
+};
+
+// Redundant specifiers should trigger a diagnostic.
+class VirtualAndOverride : public Base {
+ public:
+  virtual ~VirtualAndOverride() OVERRIDE {}
+  virtual void F() OVERRIDE {}
+};
+
+class VirtualAndFinal : public Base {
+ public:
+  virtual ~VirtualAndFinal() FINAL {}
+  virtual void F() FINAL {}
+};
+
+class VirtualAndOverrideFinal : public Base {
+ public:
+  virtual ~VirtualAndOverrideFinal() OVERRIDE FINAL {}
+  virtual void F() OVERRIDE FINAL {}
+};
+
+class OverrideAndFinal : public Base {
+ public:
+  ~OverrideAndFinal() OVERRIDE FINAL {}
+  void F() OVERRIDE FINAL {}
+};

tools/clang/plugins/tests/virtual_specifiers.flags

+-Xclang -plugin-arg-find-bad-constructs -Xclang strict-virtual-specifiers

tools/clang/plugins/tests/virtual_specifiers.txt

+virtual_specifiers.cpp:36:21: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
+  ~MissingOverride() {}
+                    ^
+                     override
+virtual_specifiers.cpp:37:11: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
+  void F() {}
+          ^
+           override
+virtual_specifiers.cpp:43:3: warning: [chromium-style] 'virtual' is redundant; 'override' implies 'virtual'.
+  virtual ~VirtualAndOverride() OVERRIDE {}
+  ^~~~~~~~
+virtual_specifiers.cpp:44:3: warning: [chromium-style] 'virtual' is redundant; 'override' implies 'virtual'.
+  virtual void F() OVERRIDE {}
+  ^~~~~~~~
+virtual_specifiers.cpp:49:3: warning: [chromium-style] 'virtual' is redundant; 'final' implies 'virtual'.
+  virtual ~VirtualAndFinal() FINAL {}
+  ^~~~~~~~
+virtual_specifiers.cpp:50:3: warning: [chromium-style] 'virtual' is redundant; 'final' implies 'virtual'.
+  virtual void F() FINAL {}
+  ^~~~~~~~
+virtual_specifiers.cpp:55:3: warning: [chromium-style] 'virtual' is redundant; 'override' implies 'virtual'.
+  virtual ~VirtualAndOverrideFinal() OVERRIDE FINAL {}
+  ^~~~~~~~
+virtual_specifiers.cpp:55:38: warning: [chromium-style] 'override' is redundant; 'final' implies 'override'.
+  virtual ~VirtualAndOverrideFinal() OVERRIDE FINAL {}
+                                     ^~~~~~~~~
+virtual_specifiers.cpp:10:18: note: expanded from macro 'OVERRIDE'
+#define OVERRIDE override
+                 ^
+virtual_specifiers.cpp:56:3: warning: [chromium-style] 'virtual' is redundant; 'override' implies 'virtual'.
+  virtual void F() OVERRIDE FINAL {}
+  ^~~~~~~~
+virtual_specifiers.cpp:56:20: warning: [chromium-style] 'override' is redundant; 'final' implies 'override'.
+  virtual void F() OVERRIDE FINAL {}
+                   ^~~~~~~~~
+virtual_specifiers.cpp:10:18: note: expanded from macro 'OVERRIDE'
+#define OVERRIDE override
+                 ^
+virtual_specifiers.cpp:61:23: warning: [chromium-style] 'override' is redundant; 'final' implies 'override'.
+  ~OverrideAndFinal() OVERRIDE FINAL {}
+                      ^~~~~~~~~
+virtual_specifiers.cpp:10:18: note: expanded from macro 'OVERRIDE'
+#define OVERRIDE override
+                 ^
+virtual_specifiers.cpp:62:12: warning: [chromium-style] 'override' is redundant; 'final' implies 'override'.
+  void F() OVERRIDE FINAL {}
+           ^~~~~~~~~
+virtual_specifiers.cpp:10:18: note: expanded from macro 'OVERRIDE'
+#define OVERRIDE override
+                 ^
+12 warnings generated.