Suppress plugin diagnostics for redundant 'virtual' keyword in system macros.

On Windows (and probably other platforms), there are some commonly used
macros to declare/define methods that include the 'virtual' keyword. It's not
useful to emit a diagnostic for these instances, since they can't be fixed
anyway.

BUG=478993
R=hans@chromium.org

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

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

7 files changed, 115 insertions, 84 deletions

diff --git a/tools/clang/plugins/ChromeClassTester.cpp b/tools/clang/plugins/ChromeClassTester.cpp
index b9d49c3..931da02 100644
--- a/tools/clang/plugins/ChromeClassTester.cpp
+++ b/tools/clang/plugins/ChromeClassTester.cpp
@@ -107,6 +107,55 @@ void ChromeClassTester::emitWarning(SourceLocation loc,
   DiagnosticBuilder builder = diagnostic().Report(full, id);
 }
 
+bool ChromeClassTester::InBannedDirectory(SourceLocation loc) {
+  if (instance().getSourceManager().isInSystemHeader(loc))
+    return true;
+
+  std::string filename;
+  if (!GetFilename(loc, &filename)) {
+    // If the filename cannot be determined, simply treat this as a banned
+    // location, instead of going through the full lookup process.
+    return true;
+  }
+
+  // We need to special case scratch space; which is where clang does its
+  // macro expansion. We explicitly want to allow people to do otherwise bad
+  // things through macros that were defined due to third party libraries.
+  if (filename == "<scratch space>")
+    return true;
+
+  // Don't complain about autogenerated protobuf files.
+  if (ends_with(filename, ".pb.h")) {
+    return true;
+  }
+
+#if defined(LLVM_ON_UNIX)
+  // We need to munge the paths so that they are relative to the repository
+  // srcroot. We first resolve the symlinktastic relative path and then
+  // remove our known srcroot from it if needed.
+  char resolvedPath[MAXPATHLEN];
+  if (realpath(filename.c_str(), resolvedPath)) {
+    filename = resolvedPath;
+  }
+#endif
+
+#if defined(LLVM_ON_WIN32)
+  std::replace(filename.begin(), filename.end(), '\\', '/');
+#endif
+
+  for (const std::string& banned_dir : banned_directories_) {
+    // If any of the banned directories occur as a component in filename,
+    // this file is rejected.
+    assert(banned_dir.front() == '/' && "Banned dir must start with '/'");
+    assert(banned_dir.back() == '/' && "Banned dir must end with '/'");
+
+    if (filename.find(banned_dir) != std::string::npos)
+      return true;
+  }
+
+  return false;
+}
+
 bool ChromeClassTester::InBannedNamespace(const Decl* record) {
   std::string n = GetNamespace(record);
   if (!n.empty()) {
@@ -237,55 +286,6 @@ std::string ChromeClassTester::GetNamespaceImpl(const DeclContext* context,
   }
 }
 
-bool ChromeClassTester::InBannedDirectory(SourceLocation loc) {
-  if (instance().getSourceManager().isInSystemHeader(loc))
-    return true;
-
-  std::string filename;
-  if (!GetFilename(loc, &filename)) {
-    // If the filename cannot be determined, simply treat this as a banned
-    // location, instead of going through the full lookup process.
-    return true;
-  }
-
-  // We need to special case scratch space; which is where clang does its
-  // macro expansion. We explicitly want to allow people to do otherwise bad
-  // things through macros that were defined due to third party libraries.
-  if (filename == "<scratch space>")
-    return true;
-
-  // Don't complain about autogenerated protobuf files.
-  if (ends_with(filename, ".pb.h")) {
-    return true;
-  }
-
-#if defined(LLVM_ON_UNIX)
-  // We need to munge the paths so that they are relative to the repository
-  // srcroot. We first resolve the symlinktastic relative path and then
-  // remove our known srcroot from it if needed.
-  char resolvedPath[MAXPATHLEN];
-  if (realpath(filename.c_str(), resolvedPath)) {
-    filename = resolvedPath;
-  }
-#endif
-
-#if defined(LLVM_ON_WIN32)
-  std::replace(filename.begin(), filename.end(), '\\', '/');
-#endif
-
-  for (const std::string& banned_dir : banned_directories_) {
-    // If any of the banned directories occur as a component in filename,
-    // this file is rejected.
-    assert(banned_dir.front() == '/' && "Banned dir must start with '/'");
-    assert(banned_dir.back() == '/' && "Banned dir must end with '/'");
-
-    if (filename.find(banned_dir) != std::string::npos)
-      return true;
-  }
-
-  return false;
-}
-
 bool ChromeClassTester::IsIgnoredType(const std::string& base_name) {
   return ignored_record_names_.find(base_name) != ignored_record_names_.end();
 }
diff --git a/tools/clang/plugins/ChromeClassTester.h b/tools/clang/plugins/ChromeClassTester.h
index 963b90f..ed65050 100644
--- a/tools/clang/plugins/ChromeClassTester.h
+++ b/tools/clang/plugins/ChromeClassTester.h
@@ -37,6 +37,10 @@ class ChromeClassTester : public clang::ASTConsumer {
   // namespace.
   bool InBannedNamespace(const clang::Decl* record);
 
+  // Utility method for subclasses to check if the source location is in a
+  // directory the plugin should ignore.
+  bool InBannedDirectory(clang::SourceLocation loc);
+
   // Utility method for subclasses to determine the namespace of the
   // specified record, if any. Unnamed namespaces will be identified as
   // "<anonymous namespace>".
@@ -64,7 +68,6 @@ class ChromeClassTester : public clang::ASTConsumer {
   // deliberately ignore) in HandleTagDeclDefinition().
   std::string GetNamespaceImpl(const clang::DeclContext* context,
                                const std::string& candidate);
-  bool InBannedDirectory(clang::SourceLocation loc);
   bool IsIgnoredType(const std::string& base_name);
 
   // Attempts to determine the filename for the given SourceLocation.
diff --git a/tools/clang/plugins/FindBadConstructsConsumer.cpp b/tools/clang/plugins/FindBadConstructsConsumer.cpp
index c7cc549..20919a4 100644
--- a/tools/clang/plugins/FindBadConstructsConsumer.cpp
+++ b/tools/clang/plugins/FindBadConstructsConsumer.cpp
@@ -70,14 +70,15 @@ bool IsGtestTestFixture(const CXXRecordDecl* decl) {
   return decl->getQualifiedNameAsString() == "testing::Test";
 }
 
+// Generates a fixit hint to remove the 'virtual' keyword.
+// Unfortunately, there doesn't seem to be a good way to determine the source
+// 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 ;-)
 FixItHint FixItRemovalForVirtual(const SourceManager& manager,
                                  const CXXMethodDecl* method) {
-  // 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 range(method->getLocStart());
   // Get the spelling loc just in case it was expanded from a macro.
   SourceRange spelling_range(manager.getSpellingLoc(range.getBegin()));
@@ -413,11 +414,17 @@ void FindBadConstructsConsumer::CheckVirtualSpecifiers(
 
   // Complain if a method is annotated virtual && (override || final).
   if (has_virtual && (override_attr || final_attr)) {
-    diagnostic().Report(method->getLocStart(),
-                        diag_redundant_virtual_specifier_)
-        << "'virtual'"
-        << (override_attr ? static_cast<Attr*>(override_attr) : final_attr)
-        << FixItRemovalForVirtual(manager, method);
+    // ... but only if virtual does not originate in a macro from a banned file.
+    // Note this is just an educated guess: the assumption here is that any
+    // macro for declaring methods will probably be at the start of the method's
+    // source range.
+    if (!InBannedDirectory(manager.getSpellingLoc(method->getLocStart()))) {
+      diagnostic().Report(method->getLocStart(),
+                          diag_redundant_virtual_specifier_)
+          << "'virtual'"
+          << (override_attr ? static_cast<Attr*>(override_attr) : final_attr)
+          << FixItRemovalForVirtual(manager, method);
+    }
   }
 
   // Complain if a method is an override and is not annotated with override or
diff --git a/tools/clang/plugins/tests/system/windows.h b/tools/clang/plugins/tests/system/windows.h
new file mode 100644
index 0000000..f93d5e2
--- /dev/null
+++ b/tools/clang/plugins/tests/system/windows.h
@@ -0,0 +1,10 @@
+// Copyright 2015 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.
+
+#ifndef TOOLS_CLANG_PLUGINS_TESTS_SYSTEM_WINDOWS_H_
+#define TOOLS_CLANG_PLUGINS_TESTS_SYSTEM_WINDOWS_H_
+
+#define STDMETHOD(x) virtual void x
+
+#endif  // TOOLS_CLANG_PLUGINS_TESTS_SYSTEM_WINDOWS_H_
diff --git a/tools/clang/plugins/tests/test.sh b/tools/clang/plugins/tests/test.sh
index ea21054..cf26252 100755
--- a/tools/clang/plugins/tests/test.sh
+++ b/tools/clang/plugins/tests/test.sh
@@ -12,6 +12,8 @@ E_FAILEDTEST=1
 
 failed_any_test=
 
+THIS_DIR="$(dirname "${0}")"
+
 # Prints usage information.
 usage() {
   echo "Usage: $(basename "${0}")" \
@@ -38,6 +40,7 @@ do_testcase() {
 
   local output="$("${CLANG_PATH}" -fsyntax-only -Wno-c++11-extensions \
       -Wno-inconsistent-missing-override \
+      -isystem ${THIS_DIR}/system \
       -Xclang -load -Xclang "${PLUGIN_PATH}" \
       -Xclang -add-plugin -Xclang find-bad-constructs ${flags} ${1} 2>&1)"
   local diffout="$(echo "${output}" | diff - "${2}")"
@@ -81,7 +84,7 @@ else
 
   # The golden files assume that the cwd is this directory. To make the script
   # work no matter what the cwd is, explicitly cd to there.
-  cd "$(dirname "${0}")"
+  cd "${THIS_DIR}"
 fi
 
 for input in *.cpp; do
diff --git a/tools/clang/plugins/tests/virtual_specifiers.cpp b/tools/clang/plugins/tests/virtual_specifiers.cpp
index 28321e8..2103248 100644
--- a/tools/clang/plugins/tests/virtual_specifiers.cpp
+++ b/tools/clang/plugins/tests/virtual_specifiers.cpp
@@ -5,6 +5,8 @@
 // Tests for chromium style checks for virtual/override/final specifiers on
 // virtual methods.
 
+#include <windows.h>
+
 // Purposely use macros to test that the FixIt hints don't try to remove the
 // macro body.
 #define OVERRIDE override
@@ -79,7 +81,13 @@ class PureVirtualOverride : public Base {
   virtual void F() override = 0;
 };
 
-// Finally, some simple sanity tests that overrides in the testing namespace
+// Test that the redundant virtual warning is suppressed when the virtual
+// keyword comes from a macro in a system header.
+class COMIsAwesome : public Base {
+  STDMETHOD(F)() override = 0;
+};
+
+// Some tests that overrides in the testing namespace
 // don't trigger warnings, except for testing::Test.
 namespace testing {
 
diff --git a/tools/clang/plugins/tests/virtual_specifiers.txt b/tools/clang/plugins/tests/virtual_specifiers.txt
index 18669af..135ebdd 100644
--- a/tools/clang/plugins/tests/virtual_specifiers.txt
+++ b/tools/clang/plugins/tests/virtual_specifiers.txt
@@ -1,69 +1,69 @@
-virtual_specifiers.cpp:36:22: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
+virtual_specifiers.cpp:38:22: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
   ~MissingOverride() {}
                      ^
                       override
-virtual_specifiers.cpp:37:12: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
+virtual_specifiers.cpp:39:12: 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_specifiers.cpp:45: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_specifiers.cpp:46: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_specifiers.cpp:51: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_specifiers.cpp:52: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_specifiers.cpp:57: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_specifiers.cpp:57:38: warning: [chromium-style] 'override' is redundant; 'final' implies 'override'.
   virtual ~VirtualAndOverrideFinal() OVERRIDE FINAL {}
                                      ^~~~~~~~~
-virtual_specifiers.cpp:10:18: note: expanded from macro 'OVERRIDE'
+virtual_specifiers.cpp:12:18: note: expanded from macro 'OVERRIDE'
 #define OVERRIDE override
                  ^
-virtual_specifiers.cpp:56:3: warning: [chromium-style] 'virtual' is redundant; 'override' implies 'virtual'.
+virtual_specifiers.cpp:58: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_specifiers.cpp:58: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'
+virtual_specifiers.cpp:12:18: note: expanded from macro 'OVERRIDE'
 #define OVERRIDE override
                  ^
-virtual_specifiers.cpp:61:23: warning: [chromium-style] 'override' is redundant; 'final' implies 'override'.
+virtual_specifiers.cpp:63:23: warning: [chromium-style] 'override' is redundant; 'final' implies 'override'.
   ~OverrideAndFinal() OVERRIDE FINAL {}
                       ^~~~~~~~~
-virtual_specifiers.cpp:10:18: note: expanded from macro 'OVERRIDE'
+virtual_specifiers.cpp:12:18: note: expanded from macro 'OVERRIDE'
 #define OVERRIDE override
                  ^
-virtual_specifiers.cpp:62:12: warning: [chromium-style] 'override' is redundant; 'final' implies 'override'.
+virtual_specifiers.cpp:64:12: warning: [chromium-style] 'override' is redundant; 'final' implies 'override'.
   void F() OVERRIDE FINAL {}
            ^~~~~~~~~
-virtual_specifiers.cpp:10:18: note: expanded from macro 'OVERRIDE'
+virtual_specifiers.cpp:12:18: note: expanded from macro 'OVERRIDE'
 #define OVERRIDE override
                  ^
-virtual_specifiers.cpp:67:20: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
+virtual_specifiers.cpp:69:20: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
   virtual void F() = 0;
                    ^
                     override
-virtual_specifiers.cpp:71:12: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
+virtual_specifiers.cpp:73:12: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
   void F() = 0;
            ^
             override
-virtual_specifiers.cpp:79:3: warning: [chromium-style] 'virtual' is redundant; 'override' implies 'virtual'.
+virtual_specifiers.cpp:81:3: warning: [chromium-style] 'virtual' is redundant; 'override' implies 'virtual'.
   virtual void F() override = 0;
   ^~~~~~~~
-virtual_specifiers.cpp:102:20: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
+virtual_specifiers.cpp:110:20: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
   virtual ~MyTest();
                    ^
                     override
-virtual_specifiers.cpp:103:3: warning: [chromium-style] 'virtual' is redundant; 'override' implies 'virtual'.
+virtual_specifiers.cpp:111:3: warning: [chromium-style] 'virtual' is redundant; 'override' implies 'virtual'.
   virtual void SetUp() override;
   ^~~~~~~~
 17 warnings generated.