Clang style plugin: add warn-only option and use it on Windows

This will allow us to flip a flag to enable warnings-as-errors once the Windows code is cleaned up enough without building a new plugin binary. Also, try not to compute the diagnostic level in multiple places. BUG=467287, 483065 Review URL: https://codereview.chromium.org/1117163002 Cr-Commit-Position: refs/heads/master@{#327852}
cbchan · May 1, 2015 · a7ae971 · a7ae971
1 parent e0ee6eb
commit a7ae971
Show file tree

Hide file tree

Showing 12 changed files with 84 additions and 30 deletions.
diff --git a/build/common.gypi b/build/common.gypi
@@ -2173,17 +2173,23 @@
                 ],
               },
               'clang_dynlib_flags%': '-Xclang -load -Xclang <(clang_lib_path) ',
+              'clang_plugin_args%': '',
             }, { # OS == "win"
               # On Windows, the plugin is built directly into clang, so there's
               # no need to load it dynamically.
               'clang_dynlib_flags%': '',
+
+              # Don't error on plugin warnings on Windows until pre-existing warnings
+              # are cleaned up.  https://crbug.com/467287
+              'clang_plugin_args%': '-Xclang -plugin-arg-find-bad-constructs -Xclang warn-only',
             }]
           ],
         },
         # If you change these, also change build/config/clang/BUILD.gn.
         'clang_chrome_plugins_flags%':
           '<(clang_dynlib_flags)'
-          '-Xclang -add-plugin -Xclang find-bad-constructs',
+          '-Xclang -add-plugin -Xclang find-bad-constructs '
+          '<(clang_plugin_args)',
       }],
       ['asan==1 or msan==1 or lsan==1 or tsan==1', {
         'clang%': 1,

diff --git a/build/config/clang/BUILD.gn b/build/config/clang/BUILD.gn
@@ -31,6 +31,16 @@ config("find_bad_constructs") {
       ]
     }
 
+    if (is_win) {
+      # Don't error on plugin warnings on Windows until pre-existing warnings
+      # are cleaned up.  https://crbug.com/467287
+      cflags += [
+        "-Xclang",
+        "-plugin-arg-find-bad-constructs",
+        "-Xclang warn-only",
+      ]
+    }
+
     cflags += [
       "-Xclang",
       "-add-plugin",

diff --git a/tools/clang/plugins/ChromeClassTester.cpp b/tools/clang/plugins/ChromeClassTester.cpp
@@ -18,6 +18,7 @@
 #endif
 
 using namespace clang;
+using chrome_checker::Options;
 
 namespace {
 
@@ -30,8 +31,10 @@ bool ends_with(const std::string& one, const std::string& two) {
 
 }  // namespace
 
-ChromeClassTester::ChromeClassTester(CompilerInstance& instance)
-    : instance_(instance),
+ChromeClassTester::ChromeClassTester(CompilerInstance& instance,
+                                     const Options& options)
+    : options_(options),
+      instance_(instance),
       diagnostic_(instance.getDiagnostics()) {
   BuildBannedLists();
 }
@@ -95,16 +98,13 @@ void ChromeClassTester::emitWarning(SourceLocation loc,
   std::string err;
   err = "[chromium-style] ";
   err += raw_error;
-  // TODO(dcheng): Re-enable -Werror for these diagnostics on Windows once all
-  // the pre-existing warnings are cleaned up. https://crbug.com/467287
-  DiagnosticIDs::Level level =
-#if !defined(LLVM_ON_WIN32)
-      diagnostic().getWarningsAsErrors() ?
-      DiagnosticIDs::Error :
-#endif
-      DiagnosticIDs::Warning;
+
+  DiagnosticIDs::Level level = getErrorLevel() == DiagnosticsEngine::Error
+      ? DiagnosticIDs::Error : DiagnosticIDs::Warning;
+
   unsigned id = diagnostic().getDiagnosticIDs()->getCustomDiagID(level, err);
   DiagnosticBuilder builder = diagnostic().Report(full, id);
+
 }
 
 bool ChromeClassTester::InBannedDirectory(SourceLocation loc) {
@@ -311,3 +311,11 @@ bool ChromeClassTester::GetFilename(SourceLocation loc,
   *filename = ploc.getFilename();
   return true;
 }
+
+DiagnosticsEngine::Level ChromeClassTester::getErrorLevel() {
+  if (options_.warn_only)
+    return DiagnosticsEngine::Warning;
+
+  return diagnostic().getWarningsAsErrors() ? DiagnosticsEngine::Error
+                                            : DiagnosticsEngine::Warning;
+}
diff --git a/tools/clang/plugins/ChromeClassTester.h b/tools/clang/plugins/ChromeClassTester.h
@@ -8,6 +8,7 @@
 #include <set>
 #include <vector>
 
+#include "Options.h"
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Frontend/CompilerInstance.h"
@@ -16,7 +17,8 @@
 // headers to subclasses which implement CheckChromeClass().
 class ChromeClassTester : public clang::ASTConsumer {
  public:
-  explicit ChromeClassTester(clang::CompilerInstance& instance);
+  ChromeClassTester(clang::CompilerInstance& instance,
+                    const chrome_checker::Options& options);
   virtual ~ChromeClassTester();
 
   // clang::ASTConsumer:
@@ -25,6 +27,8 @@ class ChromeClassTester : public clang::ASTConsumer {
 
   void CheckTag(clang::TagDecl*);
 
+  clang::DiagnosticsEngine::Level getErrorLevel();
+
  protected:
   clang::CompilerInstance& instance() { return instance_; }
   clang::DiagnosticsEngine& diagnostic() { return diagnostic_; }
@@ -50,6 +54,9 @@ class ChromeClassTester : public clang::ASTConsumer {
   // implementation (.cc, .cpp, .mm) file.
   bool InImplementationFile(clang::SourceLocation location);
 
+  // Options.
+  const chrome_checker::Options options_;
+
  private:
   void BuildBannedLists();
 

diff --git a/tools/clang/plugins/FindBadConstructsAction.cpp b/tools/clang/plugins/FindBadConstructsAction.cpp
@@ -57,6 +57,8 @@ bool FindBadConstructsAction::ParseArgs(const CompilerInstance& instance,
       options_.with_ast_visitor = true;
     } else if (args[i] == "check-templates") {
       options_.check_templates = true;
+    } else if (args[i] == "warn-only") {
+      options_.warn_only = true;
     } else {
       parsed = false;
       llvm::errs() << "Unknown clang plugin argument: " << args[i] << "\n";

diff --git a/tools/clang/plugins/FindBadConstructsConsumer.cpp b/tools/clang/plugins/FindBadConstructsConsumer.cpp
@@ -101,7 +101,7 @@ bool IsPodOrTemplateType(const CXXRecordDecl& record) {
 
 FindBadConstructsConsumer::FindBadConstructsConsumer(CompilerInstance& instance,
                                                      const Options& options)
-    : ChromeClassTester(instance), options_(options) {
+    : ChromeClassTester(instance, options) {
   // Messages for virtual method specifiers.
   diag_method_requires_override_ =
       diagnostic().getCustomDiagID(getErrorLevel(), kMethodRequiresOverride);
@@ -593,19 +593,6 @@ FindBadConstructsConsumer::CheckRecordForRefcountIssue(
   return None;
 }
 
-// Adds either a warning or error, based on the current handling of
-// -Werror.
-DiagnosticsEngine::Level FindBadConstructsConsumer::getErrorLevel() {
-#if defined(LLVM_ON_WIN32)
-  // TODO(dcheng): Re-enable -Werror for these diagnostics on Windows once all
-  // the pre-existing warnings are cleaned up. https://crbug.com/467287
-  return DiagnosticsEngine::Warning;
-#else
-  return diagnostic().getWarningsAsErrors() ? DiagnosticsEngine::Error
-                                            : DiagnosticsEngine::Warning;
-#endif
-}
-
 // Returns true if |base| specifies one of the Chromium reference counted
 // classes (base::RefCounted / base::RefCountedThreadSafe).
 bool FindBadConstructsConsumer::IsRefCountedCallback(

diff --git a/tools/clang/plugins/FindBadConstructsConsumer.h b/tools/clang/plugins/FindBadConstructsConsumer.h
@@ -71,7 +71,6 @@ class FindBadConstructsConsumer
   static RefcountIssue CheckRecordForRefcountIssue(
       const clang::CXXRecordDecl* record,
       clang::SourceLocation& loc);
-  clang::DiagnosticsEngine::Level getErrorLevel();
   static bool IsRefCountedCallback(const clang::CXXBaseSpecifier* base,
                                    clang::CXXBasePath& path,
                                    void* user_data);
@@ -86,8 +85,6 @@ class FindBadConstructsConsumer
   void CheckWeakPtrFactoryMembers(clang::SourceLocation record_location,
                                   clang::CXXRecordDecl* record);
 
-  const Options options_;
-
   unsigned diag_method_requires_override_;
   unsigned diag_redundant_virtual_specifier_;
   unsigned diag_base_method_virtual_and_final_;

diff --git a/tools/clang/plugins/Options.h b/tools/clang/plugins/Options.h
@@ -12,12 +12,14 @@ struct Options {
       : check_base_classes(false),
         check_enum_last_value(false),
         with_ast_visitor(false),
-        check_templates(false) {}
+        check_templates(false),
+        warn_only(false) {}
 
   bool check_base_classes;
   bool check_enum_last_value;
   bool with_ast_visitor;
   bool check_templates;
+  bool warn_only;
 };
 
 }  // namespace chrome_checker

diff --git a/tools/clang/plugins/tests/warn_only.cpp b/tools/clang/plugins/tests/warn_only.cpp
@@ -0,0 +1,5 @@
+// Copyright (c) 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.
+
+#include "warn_only.h"
diff --git a/tools/clang/plugins/tests/warn_only.flags b/tools/clang/plugins/tests/warn_only.flags
@@ -0,0 +1 @@
+-Werror -Xclang -plugin-arg-find-bad-constructs -Xclang warn-only
diff --git a/tools/clang/plugins/tests/warn_only.h b/tools/clang/plugins/tests/warn_only.h
@@ -0,0 +1,21 @@
+// Copyright (c) 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 WARN_ONLY_H_
+#define WARN_ONLY_H_
+
+#include <string>
+#include <vector>
+
+class InlineCtors {
+ public:
+  InlineCtors() {}
+  ~InlineCtors() {}
+
+ private:
+  std::vector<int> one_;
+  std::vector<std::string> two_;
+};
+
+#endif  // WARN_ONLY_H_
diff --git a/tools/clang/plugins/tests/warn_only.txt b/tools/clang/plugins/tests/warn_only.txt
@@ -0,0 +1,8 @@
+In file included from warn_only.cpp:5:
+./warn_only.h:13:3: warning: [chromium-style] Complex constructor has an inlined body.
+  InlineCtors() {}
+  ^
+./warn_only.h:14:3: warning: [chromium-style] Complex destructor has an inline body.
+  ~InlineCtors() {}
+  ^
+2 warnings generated.
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		-Werror -Xclang -plugin-arg-find-bad-constructs -Xclang warn-only