[clang-tools-extra] r362702 - [clang-tidy] Make the plugin honor NOLINT

Nikolai Kosjar via cfe-commits cfe-commits at lists.llvm.org
Thu Jun 6 06:13:27 PDT 2019


Author: nik
Date: Thu Jun  6 06:13:27 2019
New Revision: 362702

URL: http://llvm.org/viewvc/llvm-project?rev=362702&view=rev
Log:
[clang-tidy] Make the plugin honor NOLINT

Instantiate a ClangTidyDiagnosticConsumer also for the plugin case and
let it forward the diagnostics to the external diagnostic engine that is
already in place.

One minor difference to the clang-tidy executable case is that the
compiler checks/diagnostics are referred to with their original name.
For example, for -Wunused-variable the plugin will refer to the check as
"-Wunused-variable" while the clang-tidy executable will refer to that
as "clang-diagnostic- unused-variable". This is because the compiler
diagnostics never reach ClangTidyDiagnosticConsumer.

Differential Revision: https://reviews.llvm.org/D61487

Added:
    clang-tools-extra/trunk/test/clang-tidy/nolint-plugin.cpp
    clang-tools-extra/trunk/test/clang-tidy/nolintnextline-plugin.cpp
Modified:
    clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
    clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
    clang-tools-extra/trunk/clang-tidy/plugin/ClangTidyPlugin.cpp
    clang-tools-extra/trunk/test/clang-tidy/basic.cpp

Modified: clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp?rev=362702&r1=362701&r2=362702&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp Thu Jun  6 06:13:27 2019
@@ -275,8 +275,10 @@ std::string ClangTidyContext::getCheckNa
 }
 
 ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer(
-    ClangTidyContext &Ctx, bool RemoveIncompatibleErrors)
-    : Context(Ctx), RemoveIncompatibleErrors(RemoveIncompatibleErrors),
+    ClangTidyContext &Ctx, DiagnosticsEngine *ExternalDiagEngine,
+    bool RemoveIncompatibleErrors)
+    : Context(Ctx), ExternalDiagEngine(ExternalDiagEngine),
+      RemoveIncompatibleErrors(RemoveIncompatibleErrors),
       LastErrorRelatesToUserCode(false), LastErrorPassesLineFilter(false),
       LastErrorWasIgnored(false) {}
 
@@ -461,16 +463,22 @@ void ClangTidyDiagnosticConsumer::Handle
                         IsWarningAsError);
   }
 
-  ClangTidyDiagnosticRenderer Converter(
-      Context.getLangOpts(), &Context.DiagEngine->getDiagnosticOptions(),
-      Errors.back());
-  SmallString<100> Message;
-  Info.FormatDiagnostic(Message);
-  FullSourceLoc Loc;
-  if (Info.getLocation().isValid() && Info.hasSourceManager())
-    Loc = FullSourceLoc(Info.getLocation(), Info.getSourceManager());
-  Converter.emitDiagnostic(Loc, DiagLevel, Message, Info.getRanges(),
-                           Info.getFixItHints());
+  if (ExternalDiagEngine) {
+    // If there is an external diagnostics engine, like in the
+    // ClangTidyPluginAction case, forward the diagnostics to it.
+    forwardDiagnostic(Info);
+  } else {
+    ClangTidyDiagnosticRenderer Converter(
+        Context.getLangOpts(), &Context.DiagEngine->getDiagnosticOptions(),
+        Errors.back());
+    SmallString<100> Message;
+    Info.FormatDiagnostic(Message);
+    FullSourceLoc Loc;
+    if (Info.getLocation().isValid() && Info.hasSourceManager())
+      Loc = FullSourceLoc(Info.getLocation(), Info.getSourceManager());
+    Converter.emitDiagnostic(Loc, DiagLevel, Message, Info.getRanges(),
+                             Info.getFixItHints());
+  }
 
   if (Info.hasSourceManager())
     checkFilters(Info.getLocation(), Info.getSourceManager());
@@ -494,6 +502,68 @@ bool ClangTidyDiagnosticConsumer::passes
   return false;
 }
 
+void ClangTidyDiagnosticConsumer::forwardDiagnostic(const Diagnostic &Info) {
+  // Acquire a diagnostic ID also in the external diagnostics engine.
+  auto DiagLevelAndFormatString =
+      Context.getDiagLevelAndFormatString(Info.getID(), Info.getLocation());
+  unsigned ExternalID = ExternalDiagEngine->getDiagnosticIDs()->getCustomDiagID(
+      DiagLevelAndFormatString.first, DiagLevelAndFormatString.second);
+
+  // Forward the details.
+  auto builder = ExternalDiagEngine->Report(Info.getLocation(), ExternalID);
+  for (auto Hint : Info.getFixItHints())
+    builder << Hint;
+  for (auto Range : Info.getRanges())
+    builder << Range;
+  for (unsigned Index = 0; Index < Info.getNumArgs(); ++Index) {
+    DiagnosticsEngine::ArgumentKind kind = Info.getArgKind(Index);
+    switch (kind) {
+    case clang::DiagnosticsEngine::ak_std_string:
+      builder << Info.getArgStdStr(Index);
+      break;
+    case clang::DiagnosticsEngine::ak_c_string:
+      builder << Info.getArgCStr(Index);
+      break;
+    case clang::DiagnosticsEngine::ak_sint:
+      builder << Info.getArgSInt(Index);
+      break;
+    case clang::DiagnosticsEngine::ak_uint:
+      builder << Info.getArgUInt(Index);
+      break;
+    case clang::DiagnosticsEngine::ak_tokenkind:
+      builder << static_cast<tok::TokenKind>(Info.getRawArg(Index));
+      break;
+    case clang::DiagnosticsEngine::ak_identifierinfo:
+      builder << Info.getArgIdentifier(Index);
+      break;
+    case clang::DiagnosticsEngine::ak_qual:
+      builder << Qualifiers::fromOpaqueValue(Info.getRawArg(Index));
+      break;
+    case clang::DiagnosticsEngine::ak_qualtype:
+      builder << QualType::getFromOpaquePtr((void *)Info.getRawArg(Index));
+      break;
+    case clang::DiagnosticsEngine::ak_declarationname:
+      builder << DeclarationName::getFromOpaqueInteger(Info.getRawArg(Index));
+      break;
+    case clang::DiagnosticsEngine::ak_nameddecl:
+      builder << reinterpret_cast<const NamedDecl *>(Info.getRawArg(Index));
+      break;
+    case clang::DiagnosticsEngine::ak_nestednamespec:
+      builder << reinterpret_cast<NestedNameSpecifier *>(Info.getRawArg(Index));
+      break;
+    case clang::DiagnosticsEngine::ak_declcontext:
+      builder << reinterpret_cast<DeclContext *>(Info.getRawArg(Index));
+      break;
+    case clang::DiagnosticsEngine::ak_qualtype_pair:
+      assert(false); // This one is not passed around.
+      break;
+    case clang::DiagnosticsEngine::ak_attr:
+      builder << reinterpret_cast<Attr *>(Info.getRawArg(Index));
+      break;
+    }
+  }
+}
+
 void ClangTidyDiagnosticConsumer::checkFilters(SourceLocation Location,
                                                const SourceManager &Sources) {
   // Invalid location may mean a diagnostic in a command line, don't skip these.

Modified: clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h?rev=362702&r1=362701&r2=362702&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h (original)
+++ clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h Thu Jun  6 06:13:27 2019
@@ -190,6 +190,15 @@ public:
     return AllowEnablingAnalyzerAlphaCheckers;
   }
 
+  using DiagLevelAndFormatString = std::pair<DiagnosticIDs::Level, std::string>;
+  DiagLevelAndFormatString getDiagLevelAndFormatString(unsigned DiagnosticID,
+                                                       SourceLocation Loc) {
+    return DiagLevelAndFormatString(
+        static_cast<DiagnosticIDs::Level>(
+            DiagEngine->getDiagnosticLevel(DiagnosticID, Loc)),
+        DiagEngine->getDiagnosticIDs()->getDescription(DiagnosticID));
+  }
+
 private:
   // Writes to Stats.
   friend class ClangTidyDiagnosticConsumer;
@@ -242,6 +251,7 @@ bool ShouldSuppressDiagnostic(Diagnostic
 class ClangTidyDiagnosticConsumer : public DiagnosticConsumer {
 public:
   ClangTidyDiagnosticConsumer(ClangTidyContext &Ctx,
+                              DiagnosticsEngine *ExternalDiagEngine = nullptr,
                               bool RemoveIncompatibleErrors = true);
 
   // FIXME: The concept of converting between FixItHints and Replacements is
@@ -266,7 +276,10 @@ private:
   void checkFilters(SourceLocation Location, const SourceManager &Sources);
   bool passesLineFilter(StringRef FileName, unsigned LineNumber) const;
 
+  void forwardDiagnostic(const Diagnostic &Info);
+
   ClangTidyContext &Context;
+  DiagnosticsEngine *ExternalDiagEngine;
   bool RemoveIncompatibleErrors;
   std::vector<ClangTidyError> Errors;
   std::unique_ptr<llvm::Regex> HeaderFilter;

Modified: clang-tools-extra/trunk/clang-tidy/plugin/ClangTidyPlugin.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/plugin/ClangTidyPlugin.cpp?rev=362702&r1=362701&r2=362702&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/plugin/ClangTidyPlugin.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/plugin/ClangTidyPlugin.cpp Thu Jun  6 06:13:27 2019
@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
+#include "../ClangTidyDiagnosticConsumer.h"
 #include "../ClangTidyForceLinker.h"
 #include "../ClangTidyModule.h"
 #include "clang/Frontend/CompilerInstance.h"
@@ -19,29 +20,39 @@ namespace tidy {
 /// The core clang tidy plugin action. This just provides the AST consumer and
 /// command line flag parsing for using clang-tidy as a clang plugin.
 class ClangTidyPluginAction : public PluginASTAction {
-  /// Wrapper to grant the context the same lifetime as the action. We use
-  /// MultiplexConsumer to avoid writing out all the forwarding methods.
+  /// Wrapper to grant the context and diagnostics engine the same lifetime as
+  /// the action.
+  /// We use MultiplexConsumer to avoid writing out all the forwarding methods.
   class WrapConsumer : public MultiplexConsumer {
     std::unique_ptr<ClangTidyContext> Context;
+    std::unique_ptr<DiagnosticsEngine> DiagEngine;
 
   public:
     WrapConsumer(std::unique_ptr<ClangTidyContext> Context,
+                 std::unique_ptr<DiagnosticsEngine> DiagEngine,
                  std::vector<std::unique_ptr<ASTConsumer>> Consumer)
-        : MultiplexConsumer(std::move(Consumer)), Context(std::move(Context)) {}
+        : MultiplexConsumer(std::move(Consumer)), Context(std::move(Context)),
+          DiagEngine(std::move(DiagEngine)) {}
   };
 
 public:
   std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &Compiler,
                                                  StringRef File) override {
-    // Insert the current diagnostics engine.
-    Context->setDiagnosticsEngine(&Compiler.getDiagnostics());
+    // Create and set diagnostics engine
+    auto ExternalDiagEngine = &Compiler.getDiagnostics();
+    auto DiagConsumer =
+        new ClangTidyDiagnosticConsumer(*Context, ExternalDiagEngine);
+    auto DiagEngine = llvm::make_unique<DiagnosticsEngine>(
+        new DiagnosticIDs, new DiagnosticOptions, DiagConsumer);
+    Context->setDiagnosticsEngine(DiagEngine.get());
 
     // Create the AST consumer.
     ClangTidyASTConsumerFactory Factory(*Context);
     std::vector<std::unique_ptr<ASTConsumer>> Vec;
     Vec.push_back(Factory.CreateASTConsumer(Compiler, File));
 
-    return llvm::make_unique<WrapConsumer>(std::move(Context), std::move(Vec));
+    return llvm::make_unique<WrapConsumer>(
+        std::move(Context), std::move(DiagEngine), std::move(Vec));
   }
 
   bool ParseArgs(const CompilerInstance &,

Modified: clang-tools-extra/trunk/test/clang-tidy/basic.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/basic.cpp?rev=362702&r1=362701&r2=362702&view=diff
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/basic.cpp (original)
+++ clang-tools-extra/trunk/test/clang-tidy/basic.cpp Thu Jun  6 06:13:27 2019
@@ -1,4 +1,5 @@
 // RUN: clang-tidy %s -checks='-*,llvm-namespace-comment' -- | FileCheck %s
+// RUN: c-index-test -test-load-source-reparse 2 all %s -Xclang -add-plugin -Xclang clang-tidy -Xclang -plugin-arg-clang-tidy -Xclang -checks='-*,llvm-namespace-comment' 2>&1 | FileCheck %s
 
 namespace i {
 }

Added: clang-tools-extra/trunk/test/clang-tidy/nolint-plugin.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/nolint-plugin.cpp?rev=362702&view=auto
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/nolint-plugin.cpp (added)
+++ clang-tools-extra/trunk/test/clang-tidy/nolint-plugin.cpp Thu Jun  6 06:13:27 2019
@@ -0,0 +1,50 @@
+// REQUIRES: static-analyzer
+// RUN: c-index-test -test-load-source-reparse 2 all %s -Xclang -add-plugin -Xclang clang-tidy -Xclang -plugin-arg-clang-tidy -Xclang -checks='-*,google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult' -Wunused-variable -I%S/Inputs/nolint 2>&1 | FileCheck %s
+
+#include "trigger_warning.h"
+void I(int& Out) {
+  int In;
+  A1(In, Out);
+}
+// CHECK-NOT: trigger_warning.h:{{.*}} warning
+// CHECK-NOT: :[[@LINE-4]]:{{.*}} note
+
+class A { A(int i); };
+// CHECK-DAG: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+class B { B(int i); }; // NOLINT
+
+class C { C(int i); }; // NOLINT(for-some-other-check)
+// CHECK-DAG: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+class C1 { C1(int i); }; // NOLINT(*)
+
+class C2 { C2(int i); }; // NOLINT(not-closed-bracket-is-treated-as-skip-all
+
+class C3 { C3(int i); }; // NOLINT(google-explicit-constructor)
+
+class C4 { C4(int i); }; // NOLINT(some-check, google-explicit-constructor)
+
+class C5 { C5(int i); }; // NOLINT without-brackets-skip-all, another-check
+
+void f() {
+  int i;
+// CHECK-DAG: :[[@LINE-1]]:7: warning: unused variable 'i' [-Wunused-variable]
+//                          31:7: warning: unused variable 'i' [-Wunused-variable]
+//  int j; // NOLINT
+//  int k; // NOLINT(clang-diagnostic-unused-variable)
+}
+
+#define MACRO(X) class X { X(int i); };
+MACRO(D)
+// CHECK-DAG: :[[@LINE-1]]:7: warning: single-argument constructors must be marked explicit
+MACRO(E) // NOLINT
+
+#define MACRO_NOARG class F { F(int i); };
+MACRO_NOARG // NOLINT
+
+#define MACRO_NOLINT class G { G(int i); }; // NOLINT
+MACRO_NOLINT
+
+#define DOUBLE_MACRO MACRO(H) // NOLINT
+DOUBLE_MACRO

Added: clang-tools-extra/trunk/test/clang-tidy/nolintnextline-plugin.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/nolintnextline-plugin.cpp?rev=362702&view=auto
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/nolintnextline-plugin.cpp (added)
+++ clang-tools-extra/trunk/test/clang-tidy/nolintnextline-plugin.cpp Thu Jun  6 06:13:27 2019
@@ -0,0 +1,48 @@
+// RUN: c-index-test -test-load-source-reparse 2 all %s -Xclang -add-plugin -Xclang clang-tidy -Xclang -plugin-arg-clang-tidy -Xclang -checks='-*,google-explicit-constructor' 2>&1 | FileCheck %s
+
+class A { A(int i); };
+// CHECK: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTNEXTLINE
+class B { B(int i); };
+
+// NOLINTNEXTLINE(for-some-other-check)
+class C { C(int i); };
+// CHECK: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTNEXTLINE(*)
+class C1 { C1(int i); };
+
+// NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
+class C2 { C2(int i); };
+
+// NOLINTNEXTLINE(google-explicit-constructor)
+class C3 { C3(int i); };
+
+// NOLINTNEXTLINE(some-check, google-explicit-constructor)
+class C4 { C4(int i); };
+
+// NOLINTNEXTLINE without-brackets-skip-all, another-check
+class C5 { C5(int i); };
+
+
+// NOLINTNEXTLINE
+
+class D { D(int i); };
+// CHECK: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTNEXTLINE
+//
+class E { E(int i); };
+// CHECK: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+#define MACRO(X) class X { X(int i); };
+MACRO(F)
+// CHECK: :[[@LINE-1]]:7: warning: single-argument constructors must be marked explicit
+// NOLINTNEXTLINE
+MACRO(G)
+
+#define MACRO_NOARG class H { H(int i); };
+// NOLINTNEXTLINE
+MACRO_NOARG
+




More information about the cfe-commits mailing list