subject:"\[PATCH\] D108560\: \[clang\-tidy\] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang\-tidy warnings over multiple lines"

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

2021-09-27 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D108560#3020444 , @salman-javed-nz 
wrote:

> Also, in your example, you have begun all checks (`check`, `check2`, `check3` 
> ... `checkN`) but only ended one of them. The remaining checks are still 
> awaiting a `NOLINTEND` comment of some sort.

+1

> I say all this in the interest of keeping the Clang-Tidy code simple, and 
> reducing the amount of weird behavior that is possible (due to mistakes, lack 
> of knowledge, or "creative" use of the feature). I rather this feature be 
> strict and limited in scope, rather than flexible enough to be used in 
> unforeseen error-prone ways.

Thank you for the careful evaluation -- I agree with you!

> Perhaps this feature can be extended in the future (if need be) after it gets 
> some use and user feedback comes in?

Agreed.




Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:400-401
+  bool SuppressionIsSpecific;
+  for (const auto  : Lines) {
+if (isNOLINTFound("NOLINTBEGIN", CheckName, Line, ,
+  )) {

`List` is the same on every iteration through the loop, so we might as well set 
it once and reuse it.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:404-406
+  auto  =
+  SuppressionIsSpecific ? SpecificNolintBegins : GlobalNolintBegins;
+  List.emplace_back(LinesLoc.getLocWithOffset(NolintIndex));





Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:410-412
+  auto  =
+  SuppressionIsSpecific ? SpecificNolintBegins : GlobalNolintBegins;
+  if (!List.empty()) {





Comment at: clang-tools-extra/docs/clang-tidy/index.rst:306-308
+line*. The ``NOLINTBEGIN`` and ``NOLINTEND`` comments allow suppressing
+clang-tidy warnings on *multiple lines* (affecting all lines between the two
+comments).

We should also document the diagnostic behavior of the comments themselves (the 
fact that misusing these can generate diagnostics).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560

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

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

2021-09-24 Thread Salman Javed via Phabricator via cfe-commits

salman-javed-nz updated this revision to Diff 374969.
salman-javed-nz added a comment.

Pre-merge build error doesn't seem related to my change, but rebasing patch 
anyway just to be sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/error_in_include.inc
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/nolint_in_include.inc
  clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-at-eof.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-global-end-specific.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-global.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-without-end.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-end-at-sof.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-end-without-begin.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-mismatched-check-names.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-mismatched-delims.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-typo-in-check-name.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp
@@ -1,49 +1,51 @@
+// NOLINTNEXTLINE
 class A { A(int i); };
+
+class B { B(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 // NOLINTNEXTLINE
-class B { B(int i); };
+class C { C(int i); };
 
 // NOLINTNEXTLINE(for-some-other-check)
-class C { C(int i); };
+class D { D(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 // NOLINTNEXTLINE(*)
-class C1 { C1(int i); };
+class D1 { D1(int i); };
 
 // NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
-class C2 { C2(int i); };
+class D2 { D2(int i); };
 
 // NOLINTNEXTLINE(google-explicit-constructor)
-class C3 { C3(int i); };
+class D3 { D3(int i); };
 
 // NOLINTNEXTLINE(some-check, google-explicit-constructor)
-class C4 { C4(int i); };
+class D4 { D4(int i); };
 
 // NOLINTNEXTLINE without-brackets-skip-all, another-check
-class C5 { C5(int i); };
-
+class D5 { D5(int i); };
 
 // NOLINTNEXTLINE
 
-class D { D(int i); };
+class E { E(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 // NOLINTNEXTLINE
 //
-class E { E(int i); };
+class F { F(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 #define MACRO(X) class X { X(int i); };
-MACRO(F)
+MACRO(G)
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: single-argument constructors must be marked explicit
 // NOLINTNEXTLINE
-MACRO(G)
+MACRO(H)
 
-#define MACRO_NOARG class H { H(int i); };
+#define MACRO_NOARG class I { I(int i); };
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 9 warnings (9 NOLINT)
 
 // RUN: %check_clang_tidy %s google-explicit-constructor %t --
Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
@@ -0,0 +1,127 @@
+// RUN: %check_clang_tidy %s google-explicit-constructor %t -- --header-filter=.* -system-headers -- -isystem %S/Inputs/nolintbeginend
+
+class A { A(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN
+class B1 { B1(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTEND
+// NOLINTBEGIN
+class B2 { B2(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTBEGIN
+class B3 { B3(int i); };
+// NOLINTEND
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTBEGIN
+// NOLINTEND
+class B4 { B4(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTEND
+class B5 { B5(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN(google-explicit-constructor)
+class C1 { C1(int i); };
+// NOLINTEND(google-explicit-constructor)
+
+// NOLINTBEGIN(*)
+class C2 { C2(int i); };

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

2021-09-24 Thread Salman Javed via Phabricator via cfe-commits

salman-javed-nz updated this revision to Diff 374803.
salman-javed-nz added a comment.

`lineIsWithinNolintBegin()`:
Put `NOLINTBEGIN` comments into 2 buckets:

1. Comments that apply to a specific check - `NOLINTBEGIN(check)`
2. Comments that apply to all checks - `NOLINTBEGIN(*)` and `NOLINTBEGIN` with 
no args

If a check is begun with one type of comment, it must be ended with the same 
type.

New tests:
`nolintbeginend-begin-global-end-specific.cpp`
`nolintbeginend-begin-specific-end-global.cpp`
`nolintbeginend-mismatched-check-names.cpp`
`nolintbeginend-typo-in-check-name.cpp`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/error_in_include.inc
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/nolint_in_include.inc
  clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-at-eof.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-global-end-specific.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-global.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-without-end.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-end-at-sof.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-end-without-begin.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-mismatched-check-names.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-mismatched-delims.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-typo-in-check-name.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp
@@ -1,49 +1,51 @@
+// NOLINTNEXTLINE
 class A { A(int i); };
+
+class B { B(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 // NOLINTNEXTLINE
-class B { B(int i); };
+class C { C(int i); };
 
 // NOLINTNEXTLINE(for-some-other-check)
-class C { C(int i); };
+class D { D(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 // NOLINTNEXTLINE(*)
-class C1 { C1(int i); };
+class D1 { D1(int i); };
 
 // NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
-class C2 { C2(int i); };
+class D2 { D2(int i); };
 
 // NOLINTNEXTLINE(google-explicit-constructor)
-class C3 { C3(int i); };
+class D3 { D3(int i); };
 
 // NOLINTNEXTLINE(some-check, google-explicit-constructor)
-class C4 { C4(int i); };
+class D4 { D4(int i); };
 
 // NOLINTNEXTLINE without-brackets-skip-all, another-check
-class C5 { C5(int i); };
-
+class D5 { D5(int i); };
 
 // NOLINTNEXTLINE
 
-class D { D(int i); };
+class E { E(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 // NOLINTNEXTLINE
 //
-class E { E(int i); };
+class F { F(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 #define MACRO(X) class X { X(int i); };
-MACRO(F)
+MACRO(G)
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: single-argument constructors must be marked explicit
 // NOLINTNEXTLINE
-MACRO(G)
+MACRO(H)
 
-#define MACRO_NOARG class H { H(int i); };
+#define MACRO_NOARG class I { I(int i); };
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 9 warnings (9 NOLINT)
 
 // RUN: %check_clang_tidy %s google-explicit-constructor %t --
Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
@@ -0,0 +1,127 @@
+// RUN: %check_clang_tidy %s google-explicit-constructor %t -- --header-filter=.* -system-headers -- -isystem %S/Inputs/nolintbeginend
+
+class A { A(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN
+class B1 { B1(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTEND
+// NOLINTBEGIN
+class B2 { B2(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTBEGIN
+class B3 { B3(int i); };
+// NOLINTEND
+// NOLINTEND
+
+//

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

2021-09-24 Thread Salman Javed via Phabricator via cfe-commits

salman-javed-nz added a comment.

In D108560#3012755 , @aaron.ballman 
wrote:

> Sorry for not thinking of this sooner, but there is another diagnostic we 
> might want to consider.
>
>   // NOLINTBEGIN(check)
>   // NOLINTEND(other-check)
>
> where the file does not contain a `NOLINTEND(check)` comment anywhere. In 
> this case, the markers are not actually matched, so it's a 
> `NOLINTBEGIN(check)` comment with no end and a `NOLINTEND(other-check)` 
> comment with no begin. That seems like user confusion we'd also want to 
> diagnose, but it also could be tricky because you really want to add 
> diagnostic arguments for the check name.

See new test for this scenario in 
`test\clang-tidy\infrastructure\nolintbeginend-mismatched-check-names.cpp`.
Diagnostics are generated for both the `NOLINTBEGIN(check)` with no end and the 
`NOLINTEND(other-check)` with no begin.

> My concern here is mostly with catching typos where the user types `check` in 
> the first and `chekc` in the second

See new test in 
`test\clang-tidy\infrastructure\nolintbeginend-typo-in-check-name.cpp`.

> Relatedly, I think this construct is perhaps fine:
>
>   // NOLINTBEGIN(check)
>   // NOLINTEND(*)
>
> because the end "covers" the begin.

That was my initial feeling too. However, after doing a bit more thinking, I 
feel that this construct causes more problems than it's worth. Consider the 
following example:

  // NOLINTBEGIN(check-A)

  // NOLINTEND(*)

This seems OK, but consider what happens when we add a 4th line:

  // NOLINTBEGIN(check-A)

  // NOLINTEND(*)

...this generates a `NOLINTEND without a previous NOLINTBEGIN` diagnostic on 
line 3. From `check-B`'s perspective, it has been `END`ed on line 3 but there 
was no `BEGIN(check-B)` or `BEGIN(*)` on lines 1-2.
Which `NOLINT(BEGIN/END)` comments are acceptable on a given line depends on 
which check you're evaluating at a given moment.

This problem goes away if we don't allow the check-specific `NOLINT`s and the 
"all checks" `NOLINT`s to be mixed and matched:

Example 1:

  // NOLINTBEGIN(check-A)

  // NOLINTEND(check-A)

Example 2:

  // NOLINTBEGIN

  // NOLINTEND

Let's look at an example where auto-generated code with its own 
`NOLINT(BEGIN/END)`s is embedded in another file:

  // NOLINTBEGIN(check-A)

  /*
   *
   Place-holder for auto-generated code
   *
   */

  // NOLINTEND(check-A)

  // ...
  // ...

If the auto-generated code is as follows:

  // NOLINTBEGIN(check-B)

  // NOLINTEND

Then the complete file is:

  // NOLINTBEGIN(check-A)

  // NOLINTBEGIN(check-B)

  // NOLINTEND

  // NOLINTEND(check-A)

  // ...
  // ...

The `NOLINTEND` in the autogenerated code ends `check-B` as well as the parent 
file's `check-A`, against the user's intention.
Clang-Tidy can't offer any helpful advice without mind-reading.

> I'm a bit less clear on this though:
>
>   // NOLINTBEGIN(*)
>   // NOLINTEND(check)
>
> because the begin is not fully covered by the end. However, I'm a bit less 
> clear on what should or should not be diagnosed here, so if we wanted to 
> leave that for follow-up work, that'd be fine.

The same rule as above, //don't mix and match check-specific `NOLINT`s with 
"all checks" `NOLINT`s//, should apply here.

Also, in your example, you have begun all checks (`check`, `check2`, `check3` 
... `checkN`) but only ended one of them. The remaining checks are still 
awaiting a `NOLINTEND` comment of some sort.

I say all this in the interest of keeping the Clang-Tidy code simple, and 
reducing the amount of weird behavior that is possible (due to mistakes, lack 
of knowledge, or "creative" use of the feature). I rather this feature be 
strict and limited in scope, rather than flexible enough to be used in 
unforeseen error-prone ways.

Perhaps this feature can be extended in the future (if need be) after it gets 
some use and user feedback comes in?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560

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

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

2021-09-21 Thread Salman Javed via Phabricator via cfe-commits

salman-javed-nz added a comment.

In D108560#3012057 , @aaron.ballman 
wrote:

> Thanks, I think this is getting close! There are two more test cases that I 
> think are interesting (and should cause any issues, hopefully):
>
>   // NOLINTEND
>   // CHECK-MESSAGES: for diagnostic on the previous line
>
> and
>
>   // CHECK-MESSAGES: for diagnostic on the subsequent line
>   // NOLINTBEGIN
>
> as the only contents in the file. The idea is that we want to check that 
> searching for a "begin" when seeing an "end" at the top of the file doesn't 
> do anything silly like try to read off the start of the file, and similar for 
> "end" when seeing a "begin" at the end of a file.

The good news is that the program does not do anything silly like read off the 
boundaries of the file. :-)
What is noteworthy, however, is that in 
`test/clang-tidy/infrastructure/nolintbeginend-begin-at-eof.cpp` only the 
original clang-tidy check diag is shown, not the diag about the unmatched 
NOLINTBEGIN. This is because of the early exit in `lineIsWithinNolintBegin()`:

  // Check if there's an open NOLINT(BEGIN...END) block on the previous lines.
  // ...
  auto Error = tallyNolintBegins(Context, SM, CheckName, PrevLines,
 FileStartLoc, NolintBegins);
  // ...
  if (NolintBegins.empty())
return false;

  // Check that every block is terminated correctly on the following lines.
  // ...

This has been fixed in the patch I just posted.

As for your latest message about NOLINTBEGIN(check) ... NOLINTEND(other-check), 
I'll have a think about it and get back to you in a day or two.

Thanks!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560

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

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

2021-09-21 Thread Salman Javed via Phabricator via cfe-commits

salman-javed-nz updated this revision to Diff 373944.
salman-javed-nz marked 6 inline comments as done.
salman-javed-nz added a comment.

`lineIsWithinNolintBegin()`:

- If the search through the preceding lines returns no active `NOLINTBEGINs`, 
carry on reading the rest of the file anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/error_in_include.inc
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/nolint_in_include.inc
  clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-at-eof.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-without-end.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-end-at-sof.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-end-without-begin.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-mismatched-delims.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp
@@ -1,49 +1,51 @@
+// NOLINTNEXTLINE
 class A { A(int i); };
+
+class B { B(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 // NOLINTNEXTLINE
-class B { B(int i); };
+class C { C(int i); };
 
 // NOLINTNEXTLINE(for-some-other-check)
-class C { C(int i); };
+class D { D(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 // NOLINTNEXTLINE(*)
-class C1 { C1(int i); };
+class D1 { D1(int i); };
 
 // NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
-class C2 { C2(int i); };
+class D2 { D2(int i); };
 
 // NOLINTNEXTLINE(google-explicit-constructor)
-class C3 { C3(int i); };
+class D3 { D3(int i); };
 
 // NOLINTNEXTLINE(some-check, google-explicit-constructor)
-class C4 { C4(int i); };
+class D4 { D4(int i); };
 
 // NOLINTNEXTLINE without-brackets-skip-all, another-check
-class C5 { C5(int i); };
-
+class D5 { D5(int i); };
 
 // NOLINTNEXTLINE
 
-class D { D(int i); };
+class E { E(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 // NOLINTNEXTLINE
 //
-class E { E(int i); };
+class F { F(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 #define MACRO(X) class X { X(int i); };
-MACRO(F)
+MACRO(G)
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: single-argument constructors must be marked explicit
 // NOLINTNEXTLINE
-MACRO(G)
+MACRO(H)
 
-#define MACRO_NOARG class H { H(int i); };
+#define MACRO_NOARG class I { I(int i); };
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 9 warnings (9 NOLINT)
 
 // RUN: %check_clang_tidy %s google-explicit-constructor %t --
Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
@@ -0,0 +1,127 @@
+// RUN: %check_clang_tidy %s google-explicit-constructor %t -- --header-filter=.* -system-headers -- -isystem %S/Inputs/nolintbeginend
+
+class A { A(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN
+class B1 { B1(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTEND
+// NOLINTBEGIN
+class B2 { B2(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTBEGIN
+class B3 { B3(int i); };
+// NOLINTEND
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTBEGIN
+// NOLINTEND
+class B4 { B4(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTEND
+class B5 { B5(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN(google-explicit-constructor)
+class C1 { C1(int i); };
+// NOLINTEND(google-explicit-constructor)
+
+// NOLINTBEGIN(*)
+class C2 { C2(int i); };
+// NOLINTEND(*)
+
+// NOLINTBEGIN(some-other-check)
+class C3 { C3(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// NOLINTEND(some-other-check)
+
+// NOLINTBEGIN(some-other-check, google-explicit-constructor)

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

2021-09-21 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

Sorry for not thinking of this sooner, but there is another diagnostic we might 
want to consider.

  // NOLINTBEGIN(check)
  // NOLINTEND(other-check)

where the file does not contain a `NOLINTEND(check)` comment anywhere. In this 
case, the markers are not actually matched, so it's a `NOLINTBEGIN(check)` 
comment with no end and a `NOLINTEND(other-check)` comment with no begin. That 
seems like user confusion we'd also want to diagnose, but it also could be 
tricky because you really want to add diagnostic arguments for the check name. 
My concern here is mostly with catching typos where the user types `check` in 
the first and `chekc` in the second, so if we wanted to use the edit distance 
between what was provided and the list of check names to provide a fix-it, that 
would be very handy (and also probably difficult to implement so I definitely 
don't insist on a fix-it).

Relatedly, I think this construct is perhaps fine:

  // NOLINTBEGIN(check)
  // NOLINTEND(*)

because the end "covers" the begin. I'm a bit less clear on this though:

  // NOLINTBEGIN(*)
  // NOLINTEND(check)

because the begin is not fully covered by the end. However, I'm a bit less 
clear on what should or should not be diagnosed here, so if we wanted to leave 
that for follow-up work, that'd be fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560

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

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

2021-09-21 Thread Salman Javed via Phabricator via cfe-commits

salman-javed-nz updated this revision to Diff 373932.
salman-javed-nz added a comment.

Updated according to review comments.

- `test/clang-tidy/infrastructure/nolintbeginend-begin-at-eof.cpp`: new test
- `test/clang-tidy/infrastructure/nolintbeginend-end-at-sof.cpp`: new test
- Use `llvm::ErrorOr` return type with `getFile()`
- Split diagnostic message into 2 messages (one specific to `NOLINTBEGIN` and 
one specific to `NOLINTEND`)
- `if ... else` brace wrapping
- Spelling corrected to "nonexistent"
- Assert that `Unused` diagnostic container is empty
- change std::vector to llvm::SmallVectorImpl


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/error_in_include.inc
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/nolint_in_include.inc
  clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-at-eof.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-without-end.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-end-at-sof.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-end-without-begin.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-mismatched-delims.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp
@@ -1,49 +1,51 @@
+// NOLINTNEXTLINE
 class A { A(int i); };
+
+class B { B(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 // NOLINTNEXTLINE
-class B { B(int i); };
+class C { C(int i); };
 
 // NOLINTNEXTLINE(for-some-other-check)
-class C { C(int i); };
+class D { D(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 // NOLINTNEXTLINE(*)
-class C1 { C1(int i); };
+class D1 { D1(int i); };
 
 // NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
-class C2 { C2(int i); };
+class D2 { D2(int i); };
 
 // NOLINTNEXTLINE(google-explicit-constructor)
-class C3 { C3(int i); };
+class D3 { D3(int i); };
 
 // NOLINTNEXTLINE(some-check, google-explicit-constructor)
-class C4 { C4(int i); };
+class D4 { D4(int i); };
 
 // NOLINTNEXTLINE without-brackets-skip-all, another-check
-class C5 { C5(int i); };
-
+class D5 { D5(int i); };
 
 // NOLINTNEXTLINE
 
-class D { D(int i); };
+class E { E(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 // NOLINTNEXTLINE
 //
-class E { E(int i); };
+class F { F(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 #define MACRO(X) class X { X(int i); };
-MACRO(F)
+MACRO(G)
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: single-argument constructors must be marked explicit
 // NOLINTNEXTLINE
-MACRO(G)
+MACRO(H)
 
-#define MACRO_NOARG class H { H(int i); };
+#define MACRO_NOARG class I { I(int i); };
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 9 warnings (9 NOLINT)
 
 // RUN: %check_clang_tidy %s google-explicit-constructor %t --
Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
@@ -0,0 +1,127 @@
+// RUN: %check_clang_tidy %s google-explicit-constructor %t -- --header-filter=.* -system-headers -- -isystem %S/Inputs/nolintbeginend
+
+class A { A(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN
+class B1 { B1(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTEND
+// NOLINTBEGIN
+class B2 { B2(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTBEGIN
+class B3 { B3(int i); };
+// NOLINTEND
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTBEGIN
+// NOLINTEND
+class B4 { B4(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTEND
+class B5 { B5(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN(google-explicit-constructor)
+class C1 { C1(int i); };
+// NOLINTEND(google-explicit-constructor)
+
+//

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

2021-09-21 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

Thanks, I think this is getting close! There are two more test cases that I 
think are interesting (and should cause any issues, hopefully):

  // NOLINTEND
  // CHECK-MESSAGES: for diagnostic on the previous line

and

  // CHECK-MESSAGES: for diagnostic on the subsequent line
  // NOLINTBEGIN

as the only contents in the file. The idea is that we want to check that 
searching for a "begin" when seeing an "end" at the top of the file doesn't do 
anything silly like try to read off the start of the file, and similar for 
"end" when seeing a "begin" at the end of a file.




Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:208
+  SourceManager  = DiagEngine->getSourceManager();
+  auto File = SM.getFileManager().getFile(Error.Message.FilePath);
+  FileID ID = SM.getOrCreateFileID(*File, SrcMgr::C_User);

This helps the reader of the code to understand that `*File` below is doing 
something special (that includes an assert check, which is great).



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:367
+   Context.getCurrentBuildDirectory(), false);
+  Error.Message = tooling::DiagnosticMessage("mismatched pair of NOLINTBEGIN/"
+ "NOLINTEND comments",

This message works, but I think it'd be better if we split it into two messages:

`unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINTEND' comment`
`unmatched 'NOLINTEND' comment without a previous 'NOLINTBEGIN' comment`



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:381-387
+// Check if a new block is being started.
+if (isNOLINTFound("NOLINTBEGIN", CheckName, Line, )) {
+  NolintBegins.emplace_back(LinesLoc.getLocWithOffset(NolintIndex));
+}
+// Check if the previous block is being closed.
+else if (isNOLINTFound("NOLINTEND", CheckName, Line, )) {
+  if (!NolintBegins.empty()) {





Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:390
+  } else {
+// Trying to close a non-existant block. Return a diagnostic about this
+// misuse that can be displayed along with the original clang-tidy 
check





Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:509-510
   bool AllowIO) {
+  std::vector Unused;
+  return shouldSuppressDiagnostic(DiagLevel, Info, Context, Unused, AllowIO);
+}

Should we assert that `Unused` is empty before returning?



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:237
   const Diagnostic , ClangTidyContext 
,
+  std::vector ,
   bool AllowIO = true);

Might be better as a `SmallVectorImpl` as this should almost always be a 
container of 0 or 1 elements.



Comment at: 
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-without-end.cpp:6-8
+// Note: the expected output has been split over several lines so that 
clang-tidy
+//   does not see the "no lint" suppression comment and mistakenly assume 
it
+//   is meant for itself.

THANK YOU for these comments! :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560

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

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

2021-09-19 Thread Salman Javed via Phabricator via cfe-commits

salman-javed-nz updated this revision to Diff 373483.
salman-javed-nz added a comment.

Minor update to function signatures

- Remove arguments that are not absolutely required
- Added `const`s

This really should have been incorporated in my previous patch, so sorry about 
the double notification.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/error_in_include.inc
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/nolint_in_include.inc
  clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-without-end.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-end-without-begin.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-mismatched-delims.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp
@@ -1,49 +1,51 @@
+// NOLINTNEXTLINE
 class A { A(int i); };
+
+class B { B(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 // NOLINTNEXTLINE
-class B { B(int i); };
+class C { C(int i); };
 
 // NOLINTNEXTLINE(for-some-other-check)
-class C { C(int i); };
+class D { D(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 // NOLINTNEXTLINE(*)
-class C1 { C1(int i); };
+class D1 { D1(int i); };
 
 // NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
-class C2 { C2(int i); };
+class D2 { D2(int i); };
 
 // NOLINTNEXTLINE(google-explicit-constructor)
-class C3 { C3(int i); };
+class D3 { D3(int i); };
 
 // NOLINTNEXTLINE(some-check, google-explicit-constructor)
-class C4 { C4(int i); };
+class D4 { D4(int i); };
 
 // NOLINTNEXTLINE without-brackets-skip-all, another-check
-class C5 { C5(int i); };
-
+class D5 { D5(int i); };
 
 // NOLINTNEXTLINE
 
-class D { D(int i); };
+class E { E(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 // NOLINTNEXTLINE
 //
-class E { E(int i); };
+class F { F(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 #define MACRO(X) class X { X(int i); };
-MACRO(F)
+MACRO(G)
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: single-argument constructors must be marked explicit
 // NOLINTNEXTLINE
-MACRO(G)
+MACRO(H)
 
-#define MACRO_NOARG class H { H(int i); };
+#define MACRO_NOARG class I { I(int i); };
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 9 warnings (9 NOLINT)
 
 // RUN: %check_clang_tidy %s google-explicit-constructor %t --
Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
@@ -0,0 +1,127 @@
+// RUN: %check_clang_tidy %s google-explicit-constructor %t -- --header-filter=.* -system-headers -- -isystem %S/Inputs/nolintbeginend
+
+class A { A(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN
+class B1 { B1(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTEND
+// NOLINTBEGIN
+class B2 { B2(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTBEGIN
+class B3 { B3(int i); };
+// NOLINTEND
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTBEGIN
+// NOLINTEND
+class B4 { B4(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTEND
+class B5 { B5(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN(google-explicit-constructor)
+class C1 { C1(int i); };
+// NOLINTEND(google-explicit-constructor)
+
+// NOLINTBEGIN(*)
+class C2 { C2(int i); };
+// NOLINTEND(*)
+
+// NOLINTBEGIN(some-other-check)
+class C3 { C3(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// NOLINTEND(some-other-check)
+
+// NOLINTBEGIN(some-other-check, google-explicit-constructor)
+class C4 { C4(int i); };
+// NOLINTEND(some-other-check, google-explicit-constructor)
+
+// NOLINTBEGIN(some-other-check, google-explicit-constructor)
+//

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

2021-09-18 Thread Salman Javed via Phabricator via cfe-commits

salman-javed-nz updated this revision to Diff 373416.
salman-javed-nz marked 2 inline comments as done.
salman-javed-nz added a comment.

`shouldSuppressDiagnostic()`:

- Changed to take a container of diagnostics as an argument. If it finds any 
stray `NOLINTBEGIN`/`NOLINTEND` markers, it creates a diagnostic regarding the 
stray marker and places it in the container.

`HandleDiagnostic()`:

- Emits any diagnostics returned by `shouldSuppressDiagnostic()`.

New unit tests:

- `test\clang-tidy\infrastructure\nolintbeginend-begin-without-end.cpp`
- `test\clang-tidy\infrastructure\nolintbeginend-end-without-begin.cpp`.
- `test\clang-tidy\infrastructure\nolintbeginend-mismatched-delims.cpp`.

`IsNOLINTFound()`:

- Bug fix. `IsNOLINTFound("NOLINT", Str)` returns true when `Str` is 
`"NOLINTNEXTLINE"`. This is because the text search finds `"NOLINT"` as a 
substring of `"NOLINTNEXTLINE"`.
- Added test case in `test\clang-tidy\infrastructure\nolint.cpp`.

`LineIsMarkedWithNOLINT()`:

- Bug fix. `NOLINTNEXTLINE`s on the very first line of a file are ignored. This 
is due to `rsplit('\n\').second` returning a blank string when there are no 
more newline chars to split on.
- Added test case in `test\clang-tidy\infrastructure\nolintnextline.cpp`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/error_in_include.inc
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/nolint_in_include.inc
  clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-without-end.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-end-without-begin.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-mismatched-delims.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp
@@ -1,49 +1,51 @@
+// NOLINTNEXTLINE
 class A { A(int i); };
+
+class B { B(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 // NOLINTNEXTLINE
-class B { B(int i); };
+class C { C(int i); };
 
 // NOLINTNEXTLINE(for-some-other-check)
-class C { C(int i); };
+class D { D(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 // NOLINTNEXTLINE(*)
-class C1 { C1(int i); };
+class D1 { D1(int i); };
 
 // NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
-class C2 { C2(int i); };
+class D2 { D2(int i); };
 
 // NOLINTNEXTLINE(google-explicit-constructor)
-class C3 { C3(int i); };
+class D3 { D3(int i); };
 
 // NOLINTNEXTLINE(some-check, google-explicit-constructor)
-class C4 { C4(int i); };
+class D4 { D4(int i); };
 
 // NOLINTNEXTLINE without-brackets-skip-all, another-check
-class C5 { C5(int i); };
-
+class D5 { D5(int i); };
 
 // NOLINTNEXTLINE
 
-class D { D(int i); };
+class E { E(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 // NOLINTNEXTLINE
 //
-class E { E(int i); };
+class F { F(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 #define MACRO(X) class X { X(int i); };
-MACRO(F)
+MACRO(G)
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: single-argument constructors must be marked explicit
 // NOLINTNEXTLINE
-MACRO(G)
+MACRO(H)
 
-#define MACRO_NOARG class H { H(int i); };
+#define MACRO_NOARG class I { I(int i); };
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)
+// CHECK-MESSAGES: Suppressed 9 warnings (9 NOLINT)
 
 // RUN: %check_clang_tidy %s google-explicit-constructor %t --
Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
@@ -0,0 +1,127 @@
+// RUN: %check_clang_tidy %s google-explicit-constructor %t -- --header-filter=.* -system-headers -- -isystem %S/Inputs/nolintbeginend
+
+class A { A(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN
+class B1 { B1(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTEND
+// NOLINTBEGIN
+class B2 {

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

2021-09-18 Thread Salman Javed via Phabricator via cfe-commits

salman-javed-nz marked 5 inline comments as done.
salman-javed-nz added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp:6
+
+// NOLINTEND
+class B1 { B1(int i); };

aaron.ballman wrote:
> salman-javed-nz wrote:
> > aaron.ballman wrote:
> > > Do you think this should be diagnosed as a sign of user confusion with 
> > > the markings?
> > For a stray `NOLINTEND` like this one, I don't think so. The original 
> > warning is still raised, so I see this as clang-tidy failing safe. The user 
> > is forced to fix their mistake before the warning goes away.
> > 
> > The consequences are of the same severity as misusing the existing `NOLINT` 
> > and `NOLINTNEXTLINE` markers, e.g. putting `NOLINT` on the wrong line, or 
> > adding a blank line after `NOLINTNEXTLINE`.
> Hmm, I'm not yet convinced we don't want to diagnose this situation. I agree 
> that the behavior of *other* diagnostics is good (the user still gets those 
> diagnostics because no range has been suppressed). But it seems like the 
> programmer made a mistake if they don't balance the begin and end markers. I 
> don't think this causes major issues, but I think the code is a bit harder to 
> read because someone who spots the end marker may try looking for the begin 
> marker that doesn't exist.
> 
> I suppose there's a small chance that a stray END may surprise users for more 
> than just code readability -- consider a file with a stray end marker where 
> the user wants to lazily suppress the end file by putting `NOLINTBEGIN` at 
> the head of the file and `NOLINTEND` at the end of the file -- the stray 
> `NOLINTEND` in the middle interrupts the full range.
I see your point. Clang-Tidy should alert the user about any stray 
`NOLINTBEGIN` or `NOLINTEND` markers it sees, not leave it up to the user to 
find them themselves. Not diagnosing this is only going to create more headache 
for the user in the long run.



Comment at: 
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp:86
+
+// NOLINTBEGIN
+class H1 { H1(int i); };

aaron.ballman wrote:
> salman-javed-nz wrote:
> > aaron.ballman wrote:
> > > Should this be diagnosed as user confusion?
> > > 
> > > My concern in both of these cases isn't so much that someone writes this 
> > > intentionally, but that one of the begin/end pair gets removed 
> > > accidentally when refactoring. Helping the user to identify *where* the 
> > > unmatched delimiters are seems like it could be user-friendly behavior.
> > The consequences of this one are higher, as there is the potential to 
> > suppress warnings unintentionally and allow clang-tidy rule violations to 
> > go undetected. I agree that something more could be done here.
> > 
> > I can think of two improvements:
> > 
> > 1. In `LineIsMarkedWithNOLINT()`, when a `NOLINTBEGIN` is found, only 
> > return true if the corresponding `NOLINTEND` is found as well. Raise the 
> > original warning if the `NOLINTEND` is omitted.
> > 
> > 2. Raise an additional warning regarding the unmatched pair of delimiters. 
> > Some guidance on how to implement it would be appreciated. In the call 
> > stack of the `LineIsMarkedWithNOLINT()` function, I can't see any exposed 
> > functionality to generate new diagnostics on the fly. Would a new 
> > clang-tidy check be the place to implement this?
> That's a good question -- I don't know that I would expect 
> `LineIsMarkedWithNOLINT()` to generate a diagnostic, but it's the obvious 
> place for checking the validity of the markers. Naively, I would not expect 
> to have to run a linter to check my lint markings, I would expect the linting 
> tool to do that for me.
> 
> Would it make sense for `shouldSuppressDiagnostic()` to take a container of 
> diagnostics generated (so `LineIsMarkedWithNOLINT()` has a place to store 
> diagnostics), and `ClangTidyDiagnosticConsumer::HandleDiagnostic()` then 
> checks whether the container is empty and if not, emits the additional 
> diagnostics from there?
Thank you for the suggestion. That is exactly what I have implemented in my 
latest patch. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560

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

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

2021-09-14 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp:6
+
+// NOLINTEND
+class B1 { B1(int i); };

salman-javed-nz wrote:
> aaron.ballman wrote:
> > Do you think this should be diagnosed as a sign of user confusion with the 
> > markings?
> For a stray `NOLINTEND` like this one, I don't think so. The original warning 
> is still raised, so I see this as clang-tidy failing safe. The user is forced 
> to fix their mistake before the warning goes away.
> 
> The consequences are of the same severity as misusing the existing `NOLINT` 
> and `NOLINTNEXTLINE` markers, e.g. putting `NOLINT` on the wrong line, or 
> adding a blank line after `NOLINTNEXTLINE`.
Hmm, I'm not yet convinced we don't want to diagnose this situation. I agree 
that the behavior of *other* diagnostics is good (the user still gets those 
diagnostics because no range has been suppressed). But it seems like the 
programmer made a mistake if they don't balance the begin and end markers. I 
don't think this causes major issues, but I think the code is a bit harder to 
read because someone who spots the end marker may try looking for the begin 
marker that doesn't exist.

I suppose there's a small chance that a stray END may surprise users for more 
than just code readability -- consider a file with a stray end marker where the 
user wants to lazily suppress the end file by putting `NOLINTBEGIN` at the head 
of the file and `NOLINTEND` at the end of the file -- the stray `NOLINTEND` in 
the middle interrupts the full range.



Comment at: 
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp:86
+
+// NOLINTBEGIN
+class H1 { H1(int i); };

salman-javed-nz wrote:
> aaron.ballman wrote:
> > Should this be diagnosed as user confusion?
> > 
> > My concern in both of these cases isn't so much that someone writes this 
> > intentionally, but that one of the begin/end pair gets removed accidentally 
> > when refactoring. Helping the user to identify *where* the unmatched 
> > delimiters are seems like it could be user-friendly behavior.
> The consequences of this one are higher, as there is the potential to 
> suppress warnings unintentionally and allow clang-tidy rule violations to go 
> undetected. I agree that something more could be done here.
> 
> I can think of two improvements:
> 
> 1. In `LineIsMarkedWithNOLINT()`, when a `NOLINTBEGIN` is found, only return 
> true if the corresponding `NOLINTEND` is found as well. Raise the original 
> warning if the `NOLINTEND` is omitted.
> 
> 2. Raise an additional warning regarding the unmatched pair of delimiters. 
> Some guidance on how to implement it would be appreciated. In the call stack 
> of the `LineIsMarkedWithNOLINT()` function, I can't see any exposed 
> functionality to generate new diagnostics on the fly. Would a new clang-tidy 
> check be the place to implement this?
That's a good question -- I don't know that I would expect 
`LineIsMarkedWithNOLINT()` to generate a diagnostic, but it's the obvious place 
for checking the validity of the markers. Naively, I would not expect to have 
to run a linter to check my lint markings, I would expect the linting tool to 
do that for me.

Would it make sense for `shouldSuppressDiagnostic()` to take a container of 
diagnostics generated (so `LineIsMarkedWithNOLINT()` has a place to store 
diagnostics), and `ClangTidyDiagnosticConsumer::HandleDiagnostic()` then checks 
whether the container is empty and if not, emits the additional diagnostics 
from there?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560

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

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

2021-09-12 Thread Salman Javed via Phabricator via cfe-commits

salman-javed-nz added a comment.

In D108560#2989295 , @aaron.ballman 
wrote:

> Is this syntax used by any other tools?

It seems Google have implemented `NOLINTBEGIN` and `NOLINTEND` support in 
cpplint. I see lines such as `// NOLINTBEGIN(whitespace/line_length)`, `// 
NOLINTBEGIN(readability/check)`, `// NOLINTBEGIN(build/include)` scattered 
across different Google projects on GitHub.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560

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

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

2021-09-12 Thread Salman Javed via Phabricator via cfe-commits

salman-javed-nz updated this revision to Diff 372150.
salman-javed-nz added a comment.

Changes in this latest patch:

- `LineIsMarkedWithNOLINT()`: Moved `NOLINTBEGIN/END` aspects into a separate 
function.
- `LineIsWithinNOLINTBEGIN()`: A `NOLINTBEGIN/END` region is only considered 
valid if both the `BEGIN` and `END` markers are present. If the region is 
malformed, the original warning will be raised.

Discussion points:

- Misuse of `NOLINTBEGIN/END` now always results in the original warning being 
raised. However, a diagnostic directing the user to location of the mismatched 
`BEGIN` and `END` markers is not implemented yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/error_in_include.inc
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/nolint_in_include.inc
  clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
@@ -0,0 +1,90 @@
+// RUN: %check_clang_tidy %s google-explicit-constructor %t -- --header-filter=.* -system-headers -- -isystem %S/Inputs/nolintbeginend
+
+class A { A(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTEND
+class B1 { B1(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN
+class B2 { B2(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTEND
+// NOLINTBEGIN
+class B3 { B3(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTEND
+class B4 { B4(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN(google-explicit-constructor)
+class C1 { C1(int i); };
+// NOLINTEND(google-explicit-constructor)
+
+// NOLINTBEGIN(*)
+class C2 { C2(int i); };
+// NOLINTEND(*)
+
+// NOLINTBEGIN(some-other-check)
+class C3 { C3(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// NOLINTEND(some-other-check)
+
+// NOLINTBEGIN(some-other-check, google-explicit-constructor)
+class C4 { C4(int i); };
+// NOLINTEND(some-other-check, google-explicit-constructor)
+
+// NOLINTBEGIN(some-other-check, google-explicit-constructor)
+// NOLINTEND(some-other-check)
+class C5 { C5(int i); };
+// NOLINTEND(google-explicit-constructor)
+
+// NOLINTBEGIN(some-other-check, google-explicit-constructor)
+// NOLINTEND(google-explicit-constructor)
+class C6 { C6(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// NOLINTEND(some-other-check)
+
+// NOLINTBEGIN(google-explicit-constructor)
+// NOLINTBEGIN(some-other-check)
+class C7 { C7(int i); };
+// NOLINTEND(some-other-check)
+// NOLINTEND(google-explicit-constructor)
+
+// NOLINTBEGIN(not-closed-bracket-is-treated-as-skip-all
+class C8 { C8(int i); };
+// NOLINTEND(not-closed-bracket-is-treated-as-skip-all
+
+// NOLINTBEGIN without-brackets-skip-all, another-check
+class C9 { C9(int i); };
+// NOLINTEND without-brackets-skip-all, another-check
+
+#define MACRO(X) class X { X(int i); };
+
+MACRO(D1)
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN
+MACRO(D2)
+// NOLINTEND
+
+#define MACRO_NOARG class E { E(int i); };
+// NOLINTBEGIN
+MACRO_NOARG
+// NOLINTEND
+
+// NOLINTBEGIN
+class F { F(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+#include "error_in_include.inc"
+// CHECK-MESSAGES: error_in_include.inc:1:11: warning: single-argument constructors must be marked explicit
+
+#include "nolint_in_include.inc"
+
+// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
Index: clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/nolint_in_include.inc
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/nolint_in_include.inc
@@ -0,0 +1,3 @@
+// NOLINTBEGIN
+class H { H(int i); };
+// NOLINTEND
Index: clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/error_in_include.inc
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/error_in_include.inc
@@ -0,0 +1 @@
+class G { G(int i); };
Index: clang-tools-extra/docs/clang-tidy/index.rst

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

2021-09-10 Thread Salman Javed via Phabricator via cfe-commits

salman-javed-nz added inline comments.

Comment at: 
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp:6
+
+// NOLINTEND
+class B1 { B1(int i); };

aaron.ballman wrote:
> Do you think this should be diagnosed as a sign of user confusion with the 
> markings?
For a stray `NOLINTEND` like this one, I don't think so. The original warning 
is still raised, so I see this as clang-tidy failing safe. The user is forced 
to fix their mistake before the warning goes away.

The consequences are of the same severity as misusing the existing `NOLINT` and 
`NOLINTNEXTLINE` markers, e.g. putting `NOLINT` on the wrong line, or adding a 
blank line after `NOLINTNEXTLINE`.

Comment at: 
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp:86
+
+// NOLINTBEGIN
+class H1 { H1(int i); };

aaron.ballman wrote:
> Should this be diagnosed as user confusion?
> 
> My concern in both of these cases isn't so much that someone writes this 
> intentionally, but that one of the begin/end pair gets removed accidentally 
> when refactoring. Helping the user to identify *where* the unmatched 
> delimiters are seems like it could be user-friendly behavior.
The consequences of this one are higher, as there is the potential to suppress 
warnings unintentionally and allow clang-tidy rule violations to go undetected. 
I agree that something more could be done here.

I can think of two improvements:

1. In `LineIsMarkedWithNOLINT()`, when a `NOLINTBEGIN` is found, only return 
true if the corresponding `NOLINTEND` is found as well. Raise the original 
warning if the `NOLINTEND` is omitted.

2. Raise an additional warning regarding the unmatched pair of delimiters. Some 
guidance on how to implement it would be appreciated. In the call stack of the 
`LineIsMarkedWithNOLINT()` function, I can't see any exposed functionality to 
generate new diagnostics on the fly. Would a new clang-tidy check be the place 
to implement this?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560

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

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

2021-09-08 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

Is this syntax used by any other tools?




Comment at: 
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp:6
+
+// NOLINTEND
+class B1 { B1(int i); };

Do you think this should be diagnosed as a sign of user confusion with the 
markings?



Comment at: 
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp:86
+
+// NOLINTBEGIN
+class H1 { H1(int i); };

Should this be diagnosed as user confusion?

My concern in both of these cases isn't so much that someone writes this 
intentionally, but that one of the begin/end pair gets removed accidentally 
when refactoring. Helping the user to identify *where* the unmatched delimiters 
are seems like it could be user-friendly behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560

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

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

2021-09-08 Thread Salman Javed via Phabricator via cfe-commits

salman-javed-nz updated this revision to Diff 371308.
salman-javed-nz added a comment.

clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp:

- Added test to check what happens when NOLINTEND marker is used before 
NOLINTBEGIN marker (class B1 , line 7).
  - Warning is still displayed (NOLINTEND ends suppression but this is 
redundant as there was no suppression in the first place).

- Added test to check what happens when NOLINTBEGIN marker is used but no 
NOLINTEND marker is used afterwards (classes H1 , 
H2 ; lines 87, 88).
  - Warning is suppressed for the remainder of the file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/error_in_include.inc
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/nolint_in_include.inc
  clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
@@ -0,0 +1,90 @@
+// RUN: %check_clang_tidy %s google-explicit-constructor %t -- --header-filter=.* -system-headers -- -isystem %S/Inputs/nolintbeginend
+
+class A { A(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTEND
+class B1 { B1(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN
+class B2 { B2(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTEND
+// NOLINTBEGIN
+class B3 { B3(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTEND
+class B4 { B4(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN(google-explicit-constructor)
+class C1 { C1(int i); };
+// NOLINTEND(google-explicit-constructor)
+
+// NOLINTBEGIN(*)
+class C2 { C2(int i); };
+// NOLINTEND(*)
+
+// NOLINTBEGIN(some-other-check)
+class C3 { C3(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// NOLINTEND(some-other-check)
+
+// NOLINTBEGIN(some-other-check, google-explicit-constructor)
+class C4 { C4(int i); };
+// NOLINTEND(some-other-check, google-explicit-constructor)
+
+// NOLINTBEGIN(some-other-check, google-explicit-constructor)
+// NOLINTEND(some-other-check)
+class C5 { C5(int i); };
+// NOLINTEND(google-explicit-constructor)
+
+// NOLINTBEGIN(some-other-check, google-explicit-constructor)
+// NOLINTEND(google-explicit-constructor)
+class C6 { C6(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// NOLINTEND(some-other-check)
+
+// NOLINTBEGIN(google-explicit-constructor)
+// NOLINTBEGIN(some-other-check)
+class C7 { C7(int i); };
+// NOLINTEND(some-other-check)
+// NOLINTEND(google-explicit-constructor)
+
+// NOLINTBEGIN(not-closed-bracket-is-treated-as-skip-all
+class C8 { C8(int i); };
+// NOLINTEND(not-closed-bracket-is-treated-as-skip-all
+
+// NOLINTBEGIN without-brackets-skip-all, another-check
+class C9 { C9(int i); };
+// NOLINTEND without-brackets-skip-all, another-check
+
+#define MACRO(X) class X { X(int i); };
+
+MACRO(D1)
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN
+MACRO(D2)
+// NOLINTEND
+
+#define MACRO_NOARG class E { E(int i); };
+// NOLINTBEGIN
+MACRO_NOARG
+// NOLINTEND
+
+#include "error_in_include.inc"
+// CHECK-MESSAGES: warning: single-argument constructors must be marked explicit
+
+#include "nolint_in_include.inc"
+
+// NOLINTBEGIN
+class H1 { H1(int i); };
+class H2 { H2(int i); };
+
+// CHECK-MESSAGES: Suppressed 14 warnings (14 NOLINT)
Index: clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/nolint_in_include.inc
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/nolint_in_include.inc
@@ -0,0 +1,3 @@
+// NOLINTBEGIN
+class G { G(int i); };
+// NOLINTEND
Index: clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/error_in_include.inc
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/error_in_include.inc
@@ -0,0 +1 @@
+class F { F(int i); };
Index: clang-tools-extra/docs/clang-tidy/index.rst
===
---

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

2021-09-07 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp:82
+
+// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)

I'd like to see some additional test coverage that shows improper use and what 
happens.

e.g., what happens if there's a `NOLINTBEGIN` and no `NOLINTEND` marker? What 
happens if `NOLINTEND` appears before any `NOLINTBEGIN`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560

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

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

2021-09-02 Thread Salman Javed via Phabricator via cfe-commits

salman-javed-nz added subscribers: gonzalobg, toklinke.
salman-javed-nz added a comment.

@gonzalobg, @toklinke - I think you two might be interested in this because you 
have made proposals about this feature before.

gonzalobg - your Bugzilla ticket: https://bugs.llvm.org/show_bug.cgi?id=3
toklinke - your message on cfe-dev: 
https://lists.llvm.org/pipermail/cfe-dev/2019-November/063989.html

Thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560

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

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

2021-09-02 Thread Salman Javed via Phabricator via cfe-commits

salman-javed-nz updated this revision to Diff 370293.
salman-javed-nz added a comment.

Add additional checks in `test/clang-tidy/infrastructure/nolintbeginend.cpp` to 
check that error suppressions in one file are carried over when the file is 
`#included` in another file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/error_in_include.inc
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/nolint_in_include.inc
  clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
@@ -0,0 +1,82 @@
+// RUN: %check_clang_tidy %s google-explicit-constructor %t -- --header-filter=.* -system-headers -- -isystem %S/Inputs/nolintbeginend
+
+class A { A(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN
+class B { B(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTEND
+// NOLINTBEGIN
+class B1 { B1(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTEND
+class B2 { B2(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN(google-explicit-constructor)
+class C { C(int i); };
+// NOLINTEND(google-explicit-constructor)
+
+// NOLINTBEGIN(*)
+class C1 { C1(int i); };
+// NOLINTEND(*)
+
+// NOLINTBEGIN(some-other-check)
+class C2 { C2(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// NOLINTEND(some-other-check)
+
+// NOLINTBEGIN(some-other-check, google-explicit-constructor)
+class C3 { C3(int i); };
+// NOLINTEND(some-other-check, google-explicit-constructor)
+
+// NOLINTBEGIN(some-other-check, google-explicit-constructor)
+// NOLINTEND(some-other-check)
+class C4 { C4(int i); };
+// NOLINTEND(google-explicit-constructor)
+
+// NOLINTBEGIN(some-other-check, google-explicit-constructor)
+// NOLINTEND(google-explicit-constructor)
+class C5 { C5(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// NOLINTEND(some-other-check)
+
+// NOLINTBEGIN(google-explicit-constructor)
+// NOLINTBEGIN(some-other-check)
+class C6 { C6(int i); };
+// NOLINTEND(some-other-check)
+// NOLINTEND(google-explicit-constructor)
+
+// NOLINTBEGIN(not-closed-bracket-is-treated-as-skip-all
+class C7 { C7(int i); };
+// NOLINTEND(not-closed-bracket-is-treated-as-skip-all
+
+// NOLINTBEGIN without-brackets-skip-all, another-check
+class C8 { C8(int i); };
+// NOLINTEND without-brackets-skip-all, another-check
+
+#define MACRO(X) class X { X(int i); };
+
+MACRO(D1)
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN
+MACRO(D2)
+// NOLINTEND
+
+#define MACRO_NOARG class E { E(int i); };
+// NOLINTBEGIN
+MACRO_NOARG
+// NOLINTEND
+
+#include "error_in_include.inc"
+// CHECK-MESSAGES: warning: single-argument constructors must be marked explicit
+
+#include "nolint_in_include.inc"
+
+// CHECK-MESSAGES: Suppressed 12 warnings (12 NOLINT)
Index: clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/nolint_in_include.inc
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/nolint_in_include.inc
@@ -0,0 +1,3 @@
+// NOLINTBEGIN
+class G { G(int i); };
+// NOLINTEND
Index: clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/error_in_include.inc
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/error_in_include.inc
@@ -0,0 +1 @@
+class F { F(int i); };
Index: clang-tools-extra/docs/clang-tidy/index.rst
===
--- clang-tools-extra/docs/clang-tidy/index.rst
+++ clang-tools-extra/docs/clang-tidy/index.rst
@@ -295,17 +295,19 @@
 
 If a specific suppression mechanism is not available for a certain warning, or
 its use is not desired for some reason, :program:`clang-tidy` has a generic
-mechanism to suppress diagnostics using ``NOLINT`` or ``NOLINTNEXTLINE``
-comments.
+mechanism to suppress diagnostics using ``NOLINT``, ``NOLINTNEXTLINE``, and
+``NOLINTBEGIN`` ... ``NOLINTEND`` comments.
 
 The ``NOLINT`` comment instructs :program:`clang-tidy` to ignore warnings on the
 *same line* (it doesn't apply to a function, a block of code or any other
 language

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

2021-08-23 Thread Salman Javed via Phabricator via cfe-commits

salman-javed-nz created this revision.
salman-javed-nz added reviewers: alexfh, aaron.ballman, njames93.
salman-javed-nz added a project: clang-tools-extra.
Herald added subscribers: arphaman, xazax.hun.
salman-javed-nz requested review of this revision.

Add support for `NOLINTBEGIN` ... `NOLINTEND` comments to suppress clang-tidy 
warnings over multiple lines. All lines between the "begin" and "end" markers 
are suppressed.
Example:

  // NOLINTBEGIN(some-check)
  // 
  // 
  // 
  // NOLINTEND(some-check)

Follows similar syntax as the `NOLINT` and `NOLINTNEXTLINE` comments that are 
already implemented, i.e. allows multiple checks to be provided in parentheses; 
suppresses all checks if the parentheses are omitted, etc.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108560

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/index.rst
  clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
@@ -0,0 +1,77 @@
+class A { A(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN
+class B { B(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTEND
+// NOLINTBEGIN
+class B1 { B1(int i); };
+// NOLINTEND
+
+// NOLINTBEGIN
+// NOLINTEND
+class B2 { B2(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN(google-explicit-constructor)
+class C { C(int i); };
+// NOLINTEND(google-explicit-constructor)
+
+// NOLINTBEGIN(*)
+class C1 { C1(int i); };
+// NOLINTEND(*)
+
+// NOLINTBEGIN(some-other-check)
+class C2 { C2(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// NOLINTEND(some-other-check)
+
+// NOLINTBEGIN(some-other-check, google-explicit-constructor)
+class C3 { C3(int i); };
+// NOLINTEND(some-other-check, google-explicit-constructor)
+
+// NOLINTBEGIN(some-other-check, google-explicit-constructor)
+// NOLINTEND(some-other-check)
+class C4 { C4(int i); };
+// NOLINTEND(google-explicit-constructor)
+
+// NOLINTBEGIN(some-other-check, google-explicit-constructor)
+// NOLINTEND(google-explicit-constructor)
+class C5 { C5(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// NOLINTEND(some-other-check)
+
+// NOLINTBEGIN(google-explicit-constructor)
+// NOLINTBEGIN(some-other-check)
+class C6 { C6(int i); };
+// NOLINTEND(some-other-check)
+// NOLINTEND(google-explicit-constructor)
+
+// NOLINTBEGIN(not-closed-bracket-is-treated-as-skip-all
+class C7 { C7(int i); };
+// NOLINTEND(not-closed-bracket-is-treated-as-skip-all
+
+// NOLINTBEGIN without-brackets-skip-all, another-check
+class C8 { C8(int i); };
+// NOLINTEND without-brackets-skip-all, another-check
+
+#define MACRO(X) class X { X(int i); };
+
+MACRO(D1)
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: single-argument constructors must be marked explicit
+
+// NOLINTBEGIN
+MACRO(D2)
+// NOLINTEND
+
+#define MACRO_NOARG class E { E(int i); };
+// NOLINTBEGIN
+MACRO_NOARG
+// NOLINTEND
+
+// CHECK-MESSAGES: Suppressed 11 warnings (11 NOLINT)
+
+// RUN: %check_clang_tidy %s google-explicit-constructor %t --
Index: clang-tools-extra/docs/clang-tidy/index.rst
===
--- clang-tools-extra/docs/clang-tidy/index.rst
+++ clang-tools-extra/docs/clang-tidy/index.rst
@@ -295,17 +295,19 @@
 
 If a specific suppression mechanism is not available for a certain warning, or
 its use is not desired for some reason, :program:`clang-tidy` has a generic
-mechanism to suppress diagnostics using ``NOLINT`` or ``NOLINTNEXTLINE``
-comments.
+mechanism to suppress diagnostics using ``NOLINT``, ``NOLINTNEXTLINE``, and
+``NOLINTBEGIN`` ... ``NOLINTEND`` comments.
 
 The ``NOLINT`` comment instructs :program:`clang-tidy` to ignore warnings on the
 *same line* (it doesn't apply to a function, a block of code or any other
 language construct, it applies to the line of code it is on). If introducing the
 comment in the same line would change the formatting in undesired way, the
 ``NOLINTNEXTLINE`` comment allows to suppress clang-tidy warnings on the *next
-line*.
+line*. The ``NOLINTBEGIN`` and ``NOLINTEND`` comments allow suppressing
+clang-tidy warnings on *multiple lines* (affecting all lines between the two
+comments).
 
-Both comments can be followed by an optional list of check names in parentheses
+All comments can be followed by an optional list of check names in parentheses
 (see below for the formal syntax).
 
 For example:
@@ -325,9 +327,16 @@
 // Silence only the specified diagnostics for the

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

22 matches

Site Navigation

Mail list logo

Footer information