[PATCH] D61487: [clang-tidy] Make the plugin honor NOLINT

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

Thanks Dmitri!


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61487/new/

https://reviews.llvm.org/D61487



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61487: [clang-tidy] Make the plugin honor NOLINT

[Nikolai Kosjar via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL362702: [clang-tidy] Make the plugin honor NOLINT (authored 
by nik, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61487/new/

https://reviews.llvm.org/D61487

Files:
  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
  clang-tools-extra/trunk/test/clang-tidy/nolint-plugin.cpp
  clang-tools-extra/trunk/test/clang-tidy/nolintnextline-plugin.cpp

Index: clang-tools-extra/trunk/test/clang-tidy/nolint-plugin.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/nolint-plugin.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/nolint-plugin.cpp
@@ -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
Index: clang-tools-extra/trunk/test/clang-tidy/nolintnextline-plugin.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/nolintnextline-plugin.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/nolintnextline-plugin.cpp
@@ -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
+
Index: clang-tools-extra/trunk/test/clang-tidy/basic.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/basic.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/basic.cpp
@@ -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 -

[PATCH] D61487: [clang-tidy] Make the plugin honor NOLINT

[Nikolai Kosjar via Phabricator via cfe-commits]

nik updated this revision to Diff 203306.
nik marked 2 inline comments as done.
nik added a comment.

Addressed inline comments.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61487/new/

https://reviews.llvm.org/D61487

Files:
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tidy/plugin/ClangTidyPlugin.cpp
  test/clang-tidy/basic.cpp
  test/clang-tidy/nolint-plugin.cpp
  test/clang-tidy/nolintnextline-plugin.cpp

Index: test/clang-tidy/nolintnextline-plugin.cpp
===
--- /dev/null
+++ test/clang-tidy/nolintnextline-plugin.cpp
@@ -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
+
Index: test/clang-tidy/nolint-plugin.cpp
===
--- /dev/null
+++ test/clang-tidy/nolint-plugin.cpp
@@ -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
Index: test/clang-tidy/basic.cpp
===
--- test/clang-tidy/basic.cpp
+++ test/clang-tidy/basic.cpp
@@ -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 {
 }
Index: clang-tidy/plugin/ClangTidyPlugin.cpp
===
--- clang-tidy/plugin/ClangTidyPlugin.cpp
+++ clang-tidy/plugin/ClangTidyPlugin.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "../ClangTidy.h"
+#include "../ClangTidyDiagnosticConsumer.h"
 #include "../ClangTidyForceLinker.h"
 #inclu

[PATCH] D61487: [clang-tidy] Make the plugin honor NOLINT

[Nikolai Kosjar via Phabricator via cfe-commits]

nik marked 4 inline comments as done.
nik added inline comments.



Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:207
   CheckNamesByDiagnosticID.try_emplace(ID, CheckName);
+  CustomDiagIdParamsByDiagnosticID.try_emplace(
+  ID, CustomDiagIdParams(Level, FormatString));

gribozavr wrote:
> Did you consider adding this query API to DiagnosticIDs instead? To me it 
> looks weird that creation of custom diag IDs is handled by one class 
> (DiagnosticIDs), and queries -- by another (ClangTidyContext).
Huch, that query API is already there. For some reason, I've missed it.

Good catch, thanks! :)



Comment at: test/clang-tidy/nolintnextline-plugin.cpp:47
+
+// 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

gribozavr wrote:
> Why not on the first line?
As this file is based on test/clang-tidy/nolint.cpp, I left it at is. But 
having it at top looks more common, so I've moved it there now.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61487/new/

https://reviews.llvm.org/D61487



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61487: [clang-tidy] Make the plugin honor NOLINT

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:472
   if (Info.hasSourceManager())
 checkFilters(Info.getLocation(), Info.getSourceManager());
 }

nik wrote:
> gribozavr wrote:
> > It seems like the `checkFilters` call should not be skipped even if we have 
> > another diagnostic engine.
> checkFilters() sets some state so that later finalizeLastError() can remove 
> diagnostics/errors from ClangTidyDiagnosticConsumer::Errors and also track 
> some statistics.
> 
> Statistics are not relevant for the pluginc case as they should not be 
> printed out for that case.
> The filtering happening in finalizeLastError() does not look relevant as the 
> plugin currently only supports the "-checks=..." argument but not the e.g. 
> header and line filter. When checks are created in 
> ClangTidyCheckFactories::createChecks, the "-checks=..." argument is honored, 
> so that the !Context.isCheckEnabled(...) in finalizeLastError() is also not 
> relevant.
> 
> I agree that checkFilters()/finalizeLastError() will be needed once the 
> plugin case should support the other filtering options (header + line 
> filter), but note that this goes beyond the scope of this change here, which 
> is NOLINT filtering.
> 
> This is all new to me, so if I miss something regarding the 
> checkFilters()/finalizeLastError() thingy, please tell me, preferably with an 
> example if possible :)
It might be not relevant right now, but that code has invariants about how 
those two functions are called. Even if things happen to work, I think this 
code is too complicated to allow breaking those invariants in some cases.



Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:207
   CheckNamesByDiagnosticID.try_emplace(ID, CheckName);
+  CustomDiagIdParamsByDiagnosticID.try_emplace(
+  ID, CustomDiagIdParams(Level, FormatString));

Did you consider adding this query API to DiagnosticIDs instead? To me it looks 
weird that creation of custom diag IDs is handled by one class (DiagnosticIDs), 
and queries -- by another (ClangTidyContext).



Comment at: test/clang-tidy/nolintnextline-plugin.cpp:47
+
+// 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

Why not on the first line?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61487/new/

https://reviews.llvm.org/D61487



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61487: [clang-tidy] Make the plugin honor NOLINT

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

Ping :)


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61487/new/

https://reviews.llvm.org/D61487



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61487: [clang-tidy] Make the plugin honor NOLINT

[Nikolai Kosjar via Phabricator via cfe-commits]

nik updated this revision to Diff 200736.
nik marked an inline comment as done.
nik added a comment.

Addressed comments.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61487/new/

https://reviews.llvm.org/D61487

Files:
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tidy/plugin/ClangTidyPlugin.cpp
  test/clang-tidy/basic.cpp
  test/clang-tidy/nolint-plugin.cpp
  test/clang-tidy/nolintnextline-plugin.cpp

Index: test/clang-tidy/nolintnextline-plugin.cpp
===
--- /dev/null
+++ test/clang-tidy/nolintnextline-plugin.cpp
@@ -0,0 +1,47 @@
+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
+
+// 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
Index: test/clang-tidy/nolint-plugin.cpp
===
--- /dev/null
+++ test/clang-tidy/nolint-plugin.cpp
@@ -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
Index: test/clang-tidy/basic.cpp
===
--- test/clang-tidy/basic.cpp
+++ test/clang-tidy/basic.cpp
@@ -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 {
 }
Index: clang-tidy/plugin/ClangTidyPlugin.cpp
===
--- clang-tidy/plugin/ClangTidyPlugin.cpp
+++ clang-tidy/plugin/ClangTidyPlugin.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "../ClangTidy.h"
+#include "../ClangTidyDiagnosticConsumer.h"
 #include "../ClangTidyForceLinker.h"
 #include "../Cl

[PATCH] D61487: [clang-tidy] Make the plugin honor NOLINT

[Nikolai Kosjar via Phabricator via cfe-commits]

nik marked 4 inline comments as done.
nik added a comment.

As I've commented on, this change is not finished. However, I've addressed the 
inline comments nevertheless.

There is one TODO left for which I would like to have an opinion.




Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:472
   if (Info.hasSourceManager())
 checkFilters(Info.getLocation(), Info.getSourceManager());
 }

gribozavr wrote:
> It seems like the `checkFilters` call should not be skipped even if we have 
> another diagnostic engine.
checkFilters() sets some state so that later finalizeLastError() can remove 
diagnostics/errors from ClangTidyDiagnosticConsumer::Errors and also track some 
statistics.

Statistics are not relevant for the pluginc case as they should not be printed 
out for that case.
The filtering happening in finalizeLastError() does not look relevant as the 
plugin currently only supports the "-checks=..." argument but not the e.g. 
header and line filter. When checks are created in 
ClangTidyCheckFactories::createChecks, the "-checks=..." argument is honored, 
so that the !Context.isCheckEnabled(...) in finalizeLastError() is also not 
relevant.

I agree that checkFilters()/finalizeLastError() will be needed once the plugin 
case should support the other filtering options (header + line filter), but 
note that this goes beyond the scope of this change here, which is NOLINT 
filtering.

This is all new to me, so if I miss something regarding the 
checkFilters()/finalizeLastError() thingy, please tell me, preferably with an 
example if possible :)


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61487/new/

https://reviews.llvm.org/D61487



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61487: [clang-tidy] Make the plugin honor NOLINT

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.



Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:457
+  forwardDiagnostic(Info);
+  return;
+  }

Indentation is 2 spaces.



Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:472
   if (Info.hasSourceManager())
 checkFilters(Info.getLocation(), Info.getSourceManager());
 }

It seems like the `checkFilters` call should not be skipped even if we have 
another diagnostic engine.



Comment at: clang-tidy/ClangTidyDiagnosticConsumer.h:197
+DiagnosticIDs::Level L;
+std::string string;
+  };

Sorry, it is unclear what this struct is, and what its members are, especially 
given that the member names are `L` and `string`...


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61487/new/

https://reviews.llvm.org/D61487



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61487: [clang-tidy] Make the plugin honor NOLINT

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

Ping.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61487/new/

https://reviews.llvm.org/D61487



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61487: [clang-tidy] Make the plugin honor NOLINT

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

Ping. alexfh?


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61487/new/

https://reviews.llvm.org/D61487



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61487: [clang-tidy] Make the plugin honor NOLINT

[Nikolai Kosjar via Phabricator via cfe-commits]

nik updated this revision to Diff 198637.
nik added a comment.

The plugin makes use of ClangTidyDiagnosticConsumer and forwards diagnostics to
the external diagnostic engine.

@alexfh: What do you think? If that looks roughly OK for you, I'll finish it.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61487/new/

https://reviews.llvm.org/D61487

Files:
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tidy/plugin/ClangTidyPlugin.cpp
  test/clang-tidy/nolint-plugin.cpp
  test/clang-tidy/nolintnextline-plugin.cpp

Index: test/clang-tidy/nolintnextline-plugin.cpp
===
--- /dev/null
+++ test/clang-tidy/nolintnextline-plugin.cpp
@@ -0,0 +1,47 @@
+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
+
+// 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
Index: test/clang-tidy/nolint-plugin.cpp
===
--- /dev/null
+++ test/clang-tidy/nolint-plugin.cpp
@@ -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
Index: clang-tidy/plugin/ClangTidyPlugin.cpp
===
--- clang-tidy/plugin/ClangTidyPlugin.cpp
+++ clang-tidy/plugin/ClangTidyPlugin.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "../ClangTidy.h"
+#include "../ClangTidyDiagnosticConsumer.h"
 #include "../ClangTidyForceLinker.h"
 #include "../ClangTidyModule.h"
 #include "clang/Frontend/CompilerInstance.h"
@@ -33,8 +34,13 @@
 public:
   std::unique_ptr CreateASTConsumer(CompilerInstance &Compiler,
  StringRef File) override {
-// Insert the current diagnostics engine.
-Context->setDiagnosticsEngine(&Compiler.getDiagnostics());
+// Create a

[PATCH] D61487: [clang-tidy] Make the plugin honor NOLINT

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

Thanks for the fast comments!

In D61487#1489308 , @alexfh wrote:

> I suspect that we're missing proper test coverage here.


True, ideally all the test scripts would also trigger the plugin case code path.

I've started with a modified copy of nolint.cpp as I wasn't able to have the 
two invocations (clang-tidy, c-index-test plugin) work with the same file. For 
example, the order of the diagnostics is different (seems solvable by appending 
using -DAG) and the c-index-test plugin case does not output "Suppressed 13 
warnings (13 NOLINT)" - is there a way to exclude the check for this output for 
the c-index-test case and still having all in the same file?

I haven't tested the plugin case with nolintnextline.cpp yet, but at least this 
one does not seem to contain the unmute case as far as I see.

> Another issue is that compiler diagnostics don't pass ClangTidyContext::diag 
> in the non-plugin use case.

Right, but how is this an issue?

> Do all the existing tests pass with your patch?

Yes.

> A better way to implement diagnostic filtering in the plugin would be to make 
> ClangTidyDiagnosticConsumer able to forward diagnostics to the external 
> diagnostics engine. It will still need some sort of a buffering though to 
> handle diagnostics and notes attached to them together.

Ahh, you suggest that the plugin should have a separate diagnostic engine and 
instantiate also a ClangTidyDiagnosticConsumer object, that forwards to the 
external one? Will check how this could work.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61487/new/

https://reviews.llvm.org/D61487



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61487: [clang-tidy] Make the plugin honor NOLINT

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Apart from NOLINT handling there's more logic in 
`ClangTidyDiagnosticConsumer::HandleDiagnostic`, which isn't properly 
transferred to `ClangTidyContext::diag` in this patch. The logic that is 
transferred seems to change the behavior w.r.t. notes that can "unmute" ignored 
warnings (see https://reviews.llvm.org/D59135#1456108). I suspect that we're 
missing proper test coverage here. Another issue is that compiler diagnostics 
don't pass ClangTidyContext::diag in the non-plugin use case. Do all the 
existing tests pass with your patch?

A better way to implement diagnostic filtering in the plugin would be to make 
ClangTidyDiagnosticConsumer able to forward diagnostics to the external 
diagnostics engine. It will still need some sort of a buffering though to 
handle diagnostics and notes attached to them together.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61487/new/

https://reviews.llvm.org/D61487



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61487: [clang-tidy] Make the plugin honor NOLINT

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

This one depends on https://reviews.llvm.org/D61486


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61487/new/

https://reviews.llvm.org/D61487



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61487: [clang-tidy] Make the plugin honor NOLINT

[Nikolai Kosjar via Phabricator via cfe-commits]

nik created this revision.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.

The clang-tidy standalone tool implements the NOLINT filtering in
ClangTidyDiagnosticConsumer::HandleDiagnostic. For the plugin case no
ClangTidyDiagnosticConsumer is set up as it would have side effects with
the already set up diagnostic consumer.

This change introduces filtering in ClangTidyContext::diag() and returns
an active dummy DiagnosticBuilder in case the check was NOLINT-ed, so
that no check needs to be adapted. The filtering in HandleDiagnostic
needs to stay there as it is still relevant for non-tidy diagnostics.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D61487

Files:
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.h
  test/clang-tidy/nolint-plugin.cpp

Index: test/clang-tidy/nolint-plugin.cpp
===
--- /dev/null
+++ test/clang-tidy/nolint-plugin.cpp
@@ -0,0 +1,49 @@
+// 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'
+  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
Index: clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -215,6 +215,8 @@
   std::string ProfilePrefix;
 
   bool AllowEnablingAnalyzerAlphaCheckers;
+
+  bool LastErrorWasIgnored;
 };
 
 /// \brief A diagnostic consumer that turns each \c Diagnostic into a
@@ -255,7 +257,6 @@
   std::unique_ptr HeaderFilter;
   bool LastErrorRelatesToUserCode;
   bool LastErrorPassesLineFilter;
-  bool LastErrorWasIgnored;
 };
 
 } // end namespace tidy
Index: clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -186,7 +186,8 @@
 bool AllowEnablingAnalyzerAlphaCheckers)
 : DiagEngine(nullptr), OptionsProvider(std::move(OptionsProvider)),
   Profile(false),
-  AllowEnablingAnalyzerAlphaCheckers(AllowEnablingAnalyzerAlphaCheckers) {
+  AllowEnablingAnalyzerAlphaCheckers(AllowEnablingAnalyzerAlphaCheckers),
+  LastErrorWasIgnored(false) {
   // Before the first translation unit we can get errors related to command-line
   // parsing, use empty string for the file name in this case.
   setCurrentFile("");
@@ -194,12 +195,112 @@
 
 ClangTidyContext::~ClangTidyContext() = default;
 
+static bool IsNOLINTFound(StringRef NolintDirectiveText, StringRef Line,
+  unsigned DiagID, const ClangTidyContext &Context) {
+  const size_t NolintIndex = Line.find(NolintDirectiveText);
+  if (NolintIndex == StringRef::npos)
+return false;
+
+  size_t BracketIndex = NolintIndex + NolintDirectiveText.size();
+  // Check if the specific checks are specified in brackets.
+  if (BracketIndex < Line.size() && Line[BracketIndex] == '(') {
+++BracketIndex;
+const size_t BracketEndIndex = Line.find(')', BracketIndex);
+if (BracketEndIndex != StringRef::npos) {
+  StringRef ChecksStr =
+  Line.substr(BracketIndex, BracketEndIndex - BracketIndex);
+  // Allow disabling all the checks with "*".
+  if (ChecksStr != "*") {
+std::string CheckName = Context.getCheckName(DiagID);
+// Allow specif