[PATCH] D118104: Make run-clang-tidy.py print the configured checks correctly

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

In D118104#3292862 , @JesApp wrote:

> Well, since this was more of source of confusion than actual incorrect 
> behaviour, I don't think there should be a test for it.
>
> In general though, I think the script is complex enough to warrant some 
> testing. That being said: I don't think they should be part of this patch. 
> Also, I'm doing this on company time and I don't think my boss would be to 
> happy if I wrote a testsuite from scratch when I just wanted to fix one bug. 
> :D

I wasn't asking about creating a new test suite from scratch. I guess I wasn't 
clear. 
The idea I had was a file in `clang-tools-extra\test\clang-tidy\infrastructure` 
with something like this:

  // RUN: %run_clang_tidy 
-checks="-*,google-explicit-constructor,modernize-use-auto" %s | FileCheck 
-check-prefix=CHECK-ENABLED-CHECKS-1 %s
  // CHECK-ENABLED-CHECKS-1: Enabled checks:
  // CHECK-ENABLED-CHECKS-1-NEXT: google-explicit-constructor
  // CHECK-ENABLED-CHECKS-1-NEXT: modernize-use-auto
  
  // RUN: %run_clang_tidy 
-checks="-*,google-explicit-constructor,modernize-use-auto" -config "Checks: 
-modernize-use-auto" %s | FileCheck -check-prefix=CHECK-ENABLED-CHECKS-2 %s
  // CHECK-ENABLED-CHECKS-2: Enabled checks:
  // CHECK-ENABLED-CHECKS-2-NEXT: google-explicit-constructor
  // CHECK-ENABLED-CHECKS-2-NOT: modernize-use-auto

Note: I haven't tested this. If it doesn't work, then it's probably a minor 
tweak away from working.

This would confirm that the "Enabled checks:" message reflects the combined 
results of `-checks` and `-config` arguments.


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

https://reviews.llvm.org/D118104

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

[PATCH] D118104: Make run-clang-tidy.py print the configured checks correctly

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

My bad. Test should have called clang-tidy with `--checks` in one test case, 
and with `--config` in the second. In both cases, the disabled check should not 
appear in the `Enabled checks:` printout.


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

https://reviews.llvm.org/D118104

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

[PATCH] D119481: run-clang-tidy: Fix infinite loop on windows

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

Just a drive-by comment to say, thank you for taking the time to make this fix. 
It's a bug I've triggered many times. Great to see it being resolved.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119481

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

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz created this revision.
salman-javed-nz added reviewers: nridge, paulaltin.
salman-javed-nz added a project: clang-tools-extra.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a project: All.
salman-javed-nz requested review of this revision.
Herald added a subscriber: cfe-commits.

5da7c04  
introduced a regression in the NOLINT macro checking loop, replacing the call 
to `getImmediateExpansionRange().getBegin()` with 
`getImmediateMacroCallerLoc()`, which has similar but subtly different 
behaviour.

The consequence is that NOLINTs within macros such as the example below were 
not being detected:

  #define CREATE_STRUCT(name)\
  struct name {/* NOLINT */  \
  int a = 0; \
  int b; \
  };
  
  CREATE_STRUCT(X)

Revert to pre-patch behaviour and add test cases to cover this issue.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126138

Files:
  clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp


Index: clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
@@ -96,6 +96,13 @@
 #define MACRO_NOLINT class G { G(int i); }; // NOLINT
 MACRO_NOLINT
 
+#define MACRO_MULTILINE_NOLINT(X) \
+class X { \
+X(int i); /* NOLINT */\
+};
+MACRO_MULTILINE_NOLINT(G1)
+MACRO_MULTILINE_NOLINT(G2)
+
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
@@ -116,4 +123,4 @@
 int array3[10];  // 
NOLINT(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
 int array4[10];  // NOLINT(*-avoid-c-arrays)
 
-// CHECK-MESSAGES: Suppressed 34 warnings (34 NOLINT)
+// CHECK-MESSAGES: Suppressed 36 warnings (36 NOLINT)
Index: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
===
--- clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
+++ clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
@@ -266,7 +266,7 @@
   return true;
 if (!DiagLoc.isMacroID())
   return false;
-DiagLoc = SrcMgr.getImmediateMacroCallerLoc(DiagLoc);
+DiagLoc = SrcMgr.getImmediateExpansionRange(DiagLoc).getBegin();
   }
   return false;
 }


Index: clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
@@ -96,6 +96,13 @@
 #define MACRO_NOLINT class G { G(int i); }; // NOLINT
 MACRO_NOLINT
 
+#define MACRO_MULTILINE_NOLINT(X) \
+class X { \
+X(int i); /* NOLINT */\
+};
+MACRO_MULTILINE_NOLINT(G1)
+MACRO_MULTILINE_NOLINT(G2)
+
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
@@ -116,4 +123,4 @@
 int array3[10];  // NOLINT(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
 int array4[10];  // NOLINT(*-avoid-c-arrays)
 
-// CHECK-MESSAGES: Suppressed 34 warnings (34 NOLINT)
+// CHECK-MESSAGES: Suppressed 36 warnings (36 NOLINT)
Index: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
===
--- clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
+++ clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
@@ -266,7 +266,7 @@
   return true;
 if (!DiagLoc.isMacroID())
   return false;
-DiagLoc = SrcMgr.getImmediateMacroCallerLoc(DiagLoc);
+DiagLoc = SrcMgr.getImmediateExpansionRange(DiagLoc).getBegin();
   }
   return false;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

F23151067: Capture.png 
This screenshot shows the code snippet from the GitHub issue, and Clang-Tidy's 
behaviour before and after this fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126138

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

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz updated this revision to Diff 431213.
salman-javed-nz edited the summary of this revision.
salman-javed-nz added a comment.

Renamed test case to better explain what it is that it's testing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126138

Files:
  clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp


Index: clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
@@ -96,6 +96,16 @@
 #define MACRO_NOLINT class G { G(int i); }; // NOLINT
 MACRO_NOLINT
 
+// Check that we can suppress diagnostics about macro arguments (as opposed to
+// diagnostics about the macro contents itself).
+#define MACRO_SUPPRESS_DIAG_FOR_ARG(X) \
+  class X {\
+X(int i); /* NOLINT */ \
+  };
+
+MACRO_SUPPRESS_DIAG_FOR_ARG(G1)
+MACRO_SUPPRESS_DIAG_FOR_ARG(G2)
+
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
@@ -116,4 +126,4 @@
 int array3[10];  // 
NOLINT(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
 int array4[10];  // NOLINT(*-avoid-c-arrays)
 
-// CHECK-MESSAGES: Suppressed 34 warnings (34 NOLINT)
+// CHECK-MESSAGES: Suppressed 36 warnings (36 NOLINT)
Index: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
===
--- clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
+++ clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
@@ -266,7 +266,7 @@
   return true;
 if (!DiagLoc.isMacroID())
   return false;
-DiagLoc = SrcMgr.getImmediateMacroCallerLoc(DiagLoc);
+DiagLoc = SrcMgr.getImmediateExpansionRange(DiagLoc).getBegin();
   }
   return false;
 }


Index: clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
@@ -96,6 +96,16 @@
 #define MACRO_NOLINT class G { G(int i); }; // NOLINT
 MACRO_NOLINT
 
+// Check that we can suppress diagnostics about macro arguments (as opposed to
+// diagnostics about the macro contents itself).
+#define MACRO_SUPPRESS_DIAG_FOR_ARG(X) \
+  class X {\
+X(int i); /* NOLINT */ \
+  };
+
+MACRO_SUPPRESS_DIAG_FOR_ARG(G1)
+MACRO_SUPPRESS_DIAG_FOR_ARG(G2)
+
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
@@ -116,4 +126,4 @@
 int array3[10];  // NOLINT(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
 int array4[10];  // NOLINT(*-avoid-c-arrays)
 
-// CHECK-MESSAGES: Suppressed 34 warnings (34 NOLINT)
+// CHECK-MESSAGES: Suppressed 36 warnings (36 NOLINT)
Index: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
===
--- clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
+++ clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
@@ -266,7 +266,7 @@
   return true;
 if (!DiagLoc.isMacroID())
   return false;
-DiagLoc = SrcMgr.getImmediateMacroCallerLoc(DiagLoc);
+DiagLoc = SrcMgr.getImmediateExpansionRange(DiagLoc).getBegin();
   }
   return false;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

The pre-merge checks are failing on the clang-format check. Not sure why it's 
complaining about the formatting of the `lit` test files - those files have 
never complied with the project style and have not been checked for style for 
as long as I remember. Has this policy changed recently?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126138

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

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

[Salman Javed via Phabricator via cfe-commits]

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

Add another test, to test the specific code sample in the GitHub issue 
(check=cppcoreguidelines-pro-type-member-init).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126138

Files:
  clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp


Index: clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
@@ -1,5 +1,5 @@
 // REQUIRES: static-analyzer
-// RUN: %check_clang_tidy %s 
google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays
 %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
+// RUN: %check_clang_tidy %s 
google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays,cppcoreguidelines-pro-type-member-init
 %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
 
 #include "trigger_warning.h"
 void I(int& Out) {
@@ -96,6 +96,23 @@
 #define MACRO_NOLINT class G { G(int i); }; // NOLINT
 MACRO_NOLINT
 
+// Check that we can suppress diagnostics about macro arguments (as opposed to
+// diagnostics about the macro contents itself).
+#define MACRO_SUPPRESS_DIAG_FOR_ARG_1(X)\
+  class X { \
+X(int i); /* NOLINT(google-explicit-constructor) */ \
+  };
+
+MACRO_SUPPRESS_DIAG_FOR_ARG_1(G1)
+
+#define MACRO_SUPPRESS_DIAG_FOR_ARG_2(X)  \
+  struct X { /* NOLINT(cppcoreguidelines-pro-type-member-init) */ \
+int a = 0;\
+int b;\
+  };
+
+MACRO_SUPPRESS_DIAG_FOR_ARG_2(G2)
+
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
@@ -116,4 +133,4 @@
 int array3[10];  // 
NOLINT(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
 int array4[10];  // NOLINT(*-avoid-c-arrays)
 
-// CHECK-MESSAGES: Suppressed 34 warnings (34 NOLINT)
+// CHECK-MESSAGES: Suppressed 36 warnings (36 NOLINT)
Index: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
===
--- clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
+++ clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
@@ -266,7 +266,7 @@
   return true;
 if (!DiagLoc.isMacroID())
   return false;
-DiagLoc = SrcMgr.getImmediateMacroCallerLoc(DiagLoc);
+DiagLoc = SrcMgr.getImmediateExpansionRange(DiagLoc).getBegin();
   }
   return false;
 }


Index: clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
@@ -1,5 +1,5 @@
 // REQUIRES: static-analyzer
-// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
+// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays,cppcoreguidelines-pro-type-member-init %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
 
 #include "trigger_warning.h"
 void I(int& Out) {
@@ -96,6 +96,23 @@
 #define MACRO_NOLINT class G { G(int i); }; // NOLINT
 MACRO_NOLINT
 
+// Check that we can suppress diagnostics about macro arguments (as opposed to
+// diagnostics about the macro contents itself).
+#define MACRO_SUPPRESS_DIAG_FOR_ARG_1(X)\
+  class X { \
+X(int i); /* NOLINT(google-explicit-constructor) */ \
+  };
+
+MACRO_SUPPRESS_DIAG_FOR_ARG_1(G1)
+
+#define MACRO_SUPPRESS_DIAG_FOR_ARG_2(X)  \
+  struct X { /* NOLINT(cppcoreguidelines-pro-type-member-init) */ \
+int a = 0;\
+int b;\
+  };
+
+MACRO_SUPPRESS_DIAG_FOR_ARG_2(G2)
+
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
@@ -116,4 +133,4 @@
 int array3[10];  // NOLINT(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
 int array4[10];  // NOLINT(*-avoid-c-arrays)
 
-// CHECK-MESSAGES: Suppressed 34 warnings (34 NOLINT)
+// CHECK-MESSAGES: Suppressed 36 warnings (36 NOLINT)
Index: clang-tools-ext

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

In D126138#3529820 , @paulaltin wrote:

> Thanks for preparing this revision @salman-javed-nz!
>
> Do you think it could be worth adding a few more test cases to cover this? It 
> turned out that this issue wasn't actually specific to multi-line macros (see 
> this comment 
> ), 
> so if the test case on line 96 was passing then it must not be 
> fully/correctly testing NOLINT for single-line macros. I guess the only way 
> to do this would be to add more checks to the nolint.cpp file, but I realise 
> that's not a trivial change.

I added a second test case to this patch, to cover the specific check mentioned 
in the GitHub ticket.
This fix is check-agnostic, so I don't think we need to add even more tests 
than the two proposed here.

I think I've addressed what everyone has discussed in this review up to this 
point. Let me know what other updates I can make to this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126138

___
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

[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

[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 { B

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

[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)
+// NOL

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

[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)
+
+// NOLI

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

[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

[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

[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

[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
+
+// NOL

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

[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

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz updated this revision to Diff 375512.
salman-javed-nz marked 4 inline comments as done.
salman-javed-nz retitled this revision from "[clang-tidy] Add support for 
NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over 
multiple lines" to "[clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` 
comments".
salman-javed-nz edited the summary of this revision.
salman-javed-nz added a comment.

Addressing review comments.

`tallyNolintBegins()`:

- Refactor so that the `&List = SuppressionIsSpecific ? SpecificNolintBegins : 
GlobalNolintBegins` line is not duplicated in 2 places.

`index.rst`:

- Added documentation on the rules regarding proper usage of 
`NOLINTBEGIN/NOLINTEND`, and the diagnostic that is raised if they aren't 
followed.

Additional changes:

Phabricator Differential summary:

- Shortened commit title
- Word-wrapped commit message body for readability


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

[PATCH] D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.



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

aaron.ballman wrote:
> `List` is the same on every iteration through the loop, so we might as well 
> set it once and reuse it.
I've refactored this so that the logic to select the appropriate list (the 
ternary operation) is only defined once. 
However, this logic still has to be called within the `for` loop, not outside, 
because `SuppressionIsSpecific` can change on every iteration of the loop.

e.g.
```
// NOLINTBEGIN(check)  <-- for loop iteration 0: SuppressionIsSpecific == true, 
 &List = SpecificNolintBegins
// NOLINTBEGIN <-- for loop iteration 1: SuppressionIsSpecific == 
false, &List = GlobalNolintBegins
// NOLINTEND   <-- for loop iteration 2: SuppressionIsSpecific == 
false, &List = GlobalNolintBegins
// NOLINTEND(check)<-- for loop iteration 3: SuppressionIsSpecific == true, 
 &List = SpecificNolintBegins
```



Comment at: clang-tools-extra/docs/clang-tidy/index.rst:367
+
+All ``NOLINTBEGIN`` comments must be paired by an equal number of ``NOLINTEND``
+comments. Moreover, a pair of comments must have matching arguments -- for

Additional documentation added here about the new diagnostic and how it is 
triggered.


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

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

Also, the pre-merge build failure is confirmed to be a problem with the build 
system and not the contents of my patch.
https://github.com/google/llvm-premerge-checks/issues/353


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

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

Yes, I will need your help to commit.

Salman Javed
m...@salmanjaved.org

Thank you very much for the review. Looking back at my initial initial 
submission a few weeks ago, I can see all the valuable improvements the review 
process has made. Hopefully people find this `NOLINTBEGIN/NOLINTEND` feature 
useful.


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

[Salman Javed via Phabricator via cfe-commits]

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

Resolving build error due to failed unit test.

On some build bots, the clang-tidy diagnostics coming from 
`error_in_include.inc` get printed BEFORE all other diagnostics, REGARDLESS of 
the location of the `#include` directive in the unit test cpp file.
On other build bots, these diagnostics are printed AFTER all other diagnostics. 
Due to this inconsistency between build bots, the `CHECK-MESSAGES` statements 
are not finding their expected messages on their expected lines.

Fixed by moving the unit tests related to including `error_in_include.inc` from 
`nolintbeginend.cpp` into its own file 
`nolintbeginend-error-within-include.cpp`.
This new file only checks for one diagnostic, so is therefore unaffected by the 
diagnostic display order.


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-error-within-include.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,122 @@
+// RUN: %check_clang_tidy %s google-explicit-constructor %t
+
+class A { A(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors

[PATCH] D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

F19296441: nolintbeginend.cpp.tmp.cpp.msg 
You can see in the attached file, that when I ran the unit test on a x86_64 
Windows 10 PC, the diagnostic from `error_in_include.inc` is printed at the 
end. However, on this this clang-s390x-linux-multistage build, it is printing 
the diagnostic from `error_in_include.inc` first:
https://lab.llvm.org/buildbot/#/builders/8/builds/1860/steps/5/logs/FAIL__Clang_Tools__nolintbeginend_cpp.


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] D112596: [clang-tidy] Correct typo in bugprone-easily-swappable-parameters

[Salman Javed via Phabricator via cfe-commits]

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

The string table `DefaultIgnoredParameterTypeSuffixes` has a typo:
`ForwardIt` is mistyped as `FowardIt`.

Correct typo and add test coverage.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112596

Files:
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore-default.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore-default.cpp
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore-default.cpp
@@ -0,0 +1,77 @@
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t --
+
+// Test that all of the default entries in the IgnoredParameterTypeSuffixes
+// option are indeed ignored.
+
+struct A {};
+
+namespace IgnoredTypes {
+using Bool = A;
+using _Bool = A;
+using it = A;
+using It = A;
+using iterator = A;
+using Iterator = A;
+using inputit = A;
+using InputIt = A;
+using forwardit = A;
+using ForwardIt = A;
+using bidirit = A;
+using BidirIt = A;
+using constiterator = A;
+using const_iterator = A;
+using Const_Iterator = A;
+using Constiterator = A;
+using ConstIterator = A;
+using RandomIt = A;
+using randomit = A;
+using random_iterator = A;
+using ReverseIt = A;
+using reverse_iterator = A;
+using reverse_const_iterator = A;
+using ConstReverseIterator = A;
+using Const_Reverse_Iterator = A;
+using const_reverse_iterator = A;
+using Constreverseiterator = A;
+using constreverseiterator = A;
+} // namespace IgnoredTypes
+
+// The types used here all have a suffix that is present in the default value 
of
+// IgnoredParameterTypeSuffixes, and should therefore be ignored:
+void f1(bool Foo, bool Bar) {}
+void f2(IgnoredTypes::Bool Foo, IgnoredTypes::Bool Bar) {}
+void f3(IgnoredTypes::_Bool Foo, IgnoredTypes::_Bool Bar) {}
+void f4(IgnoredTypes::it Foo, IgnoredTypes::it Bar) {}
+void f5(IgnoredTypes::It Foo, IgnoredTypes::It Bar) {}
+void f6(IgnoredTypes::iterator Foo, IgnoredTypes::iterator Bar) {}
+void f7(IgnoredTypes::Iterator Foo, IgnoredTypes::Iterator Bar) {}
+void f8(IgnoredTypes::inputit Foo, IgnoredTypes::inputit Bar) {}
+void f9(IgnoredTypes::InputIt Foo, IgnoredTypes::InputIt Bar) {}
+void f10(IgnoredTypes::forwardit Foo, IgnoredTypes::forwardit Bar) {}
+void f11(IgnoredTypes::ForwardIt Foo, IgnoredTypes::ForwardIt Bar) {}
+void f12(IgnoredTypes::bidirit Foo, IgnoredTypes::bidirit Bar) {}
+void f13(IgnoredTypes::BidirIt Foo, IgnoredTypes::BidirIt Bar) {}
+void f14(IgnoredTypes::constiterator Foo, IgnoredTypes::constiterator Bar) {}
+void f15(IgnoredTypes::const_iterator Foo, IgnoredTypes::const_iterator Bar) {}
+void f16(IgnoredTypes::Const_Iterator Foo, IgnoredTypes::Const_Iterator Bar) {}
+void f17(IgnoredTypes::Constiterator Foo, IgnoredTypes::Constiterator Bar) {}
+void f18(IgnoredTypes::ConstIterator Foo, IgnoredTypes::ConstIterator Bar) {}
+void f19(IgnoredTypes::RandomIt Foo, IgnoredTypes::RandomIt Bar) {}
+void f20(IgnoredTypes::randomit Foo, IgnoredTypes::randomit Bar) {}
+void f21(IgnoredTypes::random_iterator Foo, IgnoredTypes::random_iterator Bar) 
{}
+void f22(IgnoredTypes::ReverseIt Foo, IgnoredTypes::ReverseIt Bar) {}
+void f23(IgnoredTypes::reverse_iterator Foo, IgnoredTypes::reverse_iterator 
Bar) {}
+void f24(IgnoredTypes::reverse_const_iterator Foo, 
IgnoredTypes::reverse_const_iterator Bar) {}
+void f25(IgnoredTypes::ConstReverseIterator Foo, 
IgnoredTypes::ConstReverseIterator Bar) {}
+void f26(IgnoredTypes::Const_Reverse_Iterator Foo, 
IgnoredTypes::Const_Reverse_Iterator Bar) {}
+void f27(IgnoredTypes::const_reverse_iterator Foo, 
IgnoredTypes::const_reverse_iterator Bar) {}
+void f28(IgnoredTypes::Constreverseiterator Foo, 
IgnoredTypes::Constreverseiterator Bar) {}
+void f29(IgnoredTypes::constreverseiterator Foo, 
IgnoredTypes::constreverseiterator Bar) {}
+
+// This suffix of this type is not present in IgnoredParameterTypeSuffixes'
+// default value, therefore, a warning _should_ be generated.
+using ShouldNotBeIgnored = A;
+void f30(ShouldNotBeIgnored Foo, ShouldNotBeIgnored Bar) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 2 adjacent parameters of 'f30' of 
similar type ('ShouldNotBeIgnored') are easily swapped by mistake 
[bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:29: note: the first parameter in the range is 
'Foo'
+// CHECK-MESSAGES: :[[@LINE-3]]:53: note: the last parameter in the range is 
'Bar'
Index: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
===
--- clang-tools-extr

[PATCH] D112596: [clang-tidy] Correct typo in bugprone-easily-swappable-parameters

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

Pretty sure this is a typo. Curiously the unit tests did not pick it up.
I have gone through the liberty of adding a unit test to lock down the correct 
spellings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112596

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

[PATCH] D112596: [clang-tidy] Correct typo in bugprone-easily-swappable-parameters

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

The documentation looks correct.
https://clang.llvm.org/extra/clang-tidy/checks/bugprone-easily-swappable-parameters.html

> By default, the following, and their lowercase-initial variants are ignored: 
> bool, It, Iterator, InputIt, ForwardIt, BidirIt, RandomIt, random_iterator, 
> ReverseIt, reverse_iterator, reverse_const_iterator, RandomIt, 
> random_iterator, ReverseIt, reverse_iterator, reverse_const_iterator, 
> Const_Iterator, ConstIterator, const_reverse_iterator, ConstReverseIterator. 
> In addition, _Bool (but not _bool) is also part of the default value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112596

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

[PATCH] D112596: [clang-tidy] Correct typo in bugprone-easily-swappable-parameters

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

I have just received commit access, and this will be my first patch that I can 
commit on my own. 
I'll be in touch if I run into committing issues. Thanks for the review!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112596

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

[PATCH] D112596: [clang-tidy] Correct typo in bugprone-easily-swappable-parameters

[Salman Javed via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG897402e95988: [clang-tidy] Correct typo in 
bugprone-easily-swappable-parameters (authored by salman-javed-nz).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112596

Files:
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore-default.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore-default.cpp
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore-default.cpp
@@ -0,0 +1,77 @@
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t --
+
+// Test that all of the default entries in the IgnoredParameterTypeSuffixes
+// option are indeed ignored.
+
+struct A {};
+
+namespace IgnoredTypes {
+using Bool = A;
+using _Bool = A;
+using it = A;
+using It = A;
+using iterator = A;
+using Iterator = A;
+using inputit = A;
+using InputIt = A;
+using forwardit = A;
+using ForwardIt = A;
+using bidirit = A;
+using BidirIt = A;
+using constiterator = A;
+using const_iterator = A;
+using Const_Iterator = A;
+using Constiterator = A;
+using ConstIterator = A;
+using RandomIt = A;
+using randomit = A;
+using random_iterator = A;
+using ReverseIt = A;
+using reverse_iterator = A;
+using reverse_const_iterator = A;
+using ConstReverseIterator = A;
+using Const_Reverse_Iterator = A;
+using const_reverse_iterator = A;
+using Constreverseiterator = A;
+using constreverseiterator = A;
+} // namespace IgnoredTypes
+
+// The types used here all have a suffix that is present in the default value 
of
+// IgnoredParameterTypeSuffixes, and should therefore be ignored:
+void f1(bool Foo, bool Bar) {}
+void f2(IgnoredTypes::Bool Foo, IgnoredTypes::Bool Bar) {}
+void f3(IgnoredTypes::_Bool Foo, IgnoredTypes::_Bool Bar) {}
+void f4(IgnoredTypes::it Foo, IgnoredTypes::it Bar) {}
+void f5(IgnoredTypes::It Foo, IgnoredTypes::It Bar) {}
+void f6(IgnoredTypes::iterator Foo, IgnoredTypes::iterator Bar) {}
+void f7(IgnoredTypes::Iterator Foo, IgnoredTypes::Iterator Bar) {}
+void f8(IgnoredTypes::inputit Foo, IgnoredTypes::inputit Bar) {}
+void f9(IgnoredTypes::InputIt Foo, IgnoredTypes::InputIt Bar) {}
+void f10(IgnoredTypes::forwardit Foo, IgnoredTypes::forwardit Bar) {}
+void f11(IgnoredTypes::ForwardIt Foo, IgnoredTypes::ForwardIt Bar) {}
+void f12(IgnoredTypes::bidirit Foo, IgnoredTypes::bidirit Bar) {}
+void f13(IgnoredTypes::BidirIt Foo, IgnoredTypes::BidirIt Bar) {}
+void f14(IgnoredTypes::constiterator Foo, IgnoredTypes::constiterator Bar) {}
+void f15(IgnoredTypes::const_iterator Foo, IgnoredTypes::const_iterator Bar) {}
+void f16(IgnoredTypes::Const_Iterator Foo, IgnoredTypes::Const_Iterator Bar) {}
+void f17(IgnoredTypes::Constiterator Foo, IgnoredTypes::Constiterator Bar) {}
+void f18(IgnoredTypes::ConstIterator Foo, IgnoredTypes::ConstIterator Bar) {}
+void f19(IgnoredTypes::RandomIt Foo, IgnoredTypes::RandomIt Bar) {}
+void f20(IgnoredTypes::randomit Foo, IgnoredTypes::randomit Bar) {}
+void f21(IgnoredTypes::random_iterator Foo, IgnoredTypes::random_iterator Bar) 
{}
+void f22(IgnoredTypes::ReverseIt Foo, IgnoredTypes::ReverseIt Bar) {}
+void f23(IgnoredTypes::reverse_iterator Foo, IgnoredTypes::reverse_iterator 
Bar) {}
+void f24(IgnoredTypes::reverse_const_iterator Foo, 
IgnoredTypes::reverse_const_iterator Bar) {}
+void f25(IgnoredTypes::ConstReverseIterator Foo, 
IgnoredTypes::ConstReverseIterator Bar) {}
+void f26(IgnoredTypes::Const_Reverse_Iterator Foo, 
IgnoredTypes::Const_Reverse_Iterator Bar) {}
+void f27(IgnoredTypes::const_reverse_iterator Foo, 
IgnoredTypes::const_reverse_iterator Bar) {}
+void f28(IgnoredTypes::Constreverseiterator Foo, 
IgnoredTypes::Constreverseiterator Bar) {}
+void f29(IgnoredTypes::constreverseiterator Foo, 
IgnoredTypes::constreverseiterator Bar) {}
+
+// This suffix of this type is not present in IgnoredParameterTypeSuffixes'
+// default value, therefore, a warning _should_ be generated.
+using ShouldNotBeIgnored = A;
+void f30(ShouldNotBeIgnored Foo, ShouldNotBeIgnored Bar) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: 2 adjacent parameters of 'f30' of 
similar type ('ShouldNotBeIgnored') are easily swapped by mistake 
[bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:29: note: the first parameter in the range is 
'Foo'
+// CHECK-MESSAGES: :[[@LINE-3]]:53: note: the last parameter in the range is 
'Bar'
Index: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
@@ 

[PATCH] D112864: [clang-tidy] Fix lint warnings in clang-tidy source code (NFC)

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz created this revision.
salman-javed-nz added reviewers: alexfh, aaron.ballman, carlosgalvezp.
salman-javed-nz added a project: clang-tools-extra.
Herald added subscribers: abrachet, lebedev.ri, zzheng, kbarton, xazax.hun, 
nemanjai.
Herald added a reviewer: lebedev.ri.
salman-javed-nz requested review of this revision.

Run clang-tidy on all source files under `clang-tools-extra/clang-tidy`
with `-header-filter=clang-tidy.*` and make suggested corrections.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D112864

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ClangTidy.h
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/clang-tidy/ClangTidyOptions.h
  clang-tools-extra/clang-tidy/abseil/DurationDivisionCheck.h
  clang-tools-extra/clang-tidy/abseil/DurationFactoryScaleCheck.cpp
  clang-tools-extra/clang-tidy/abseil/DurationRewriter.cpp
  clang-tools-extra/clang-tidy/abseil/DurationRewriter.h
  clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp
  clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/CopyConstructorInitCheck.h
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/ForwardingReferenceOverloadCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/MultipleStatementMacroCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h
  clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SuspiciousStringCompareCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnusedRaiiCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/SlicingCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/SlicingCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h
  clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.h
  clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
  clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp
  clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
  clang-tools-extra/clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp
  clang-tools-extra/clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h
  clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
  clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
  clang-tools-extra/clang-tidy/modernize/ReplaceAutoPtrCheck.h
  
clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.h
  clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.h
  clang-tools-extra/clang-tidy/performance/ImplicitConversionInLoopCheck.cpp
  clang-tools-extra/clang-tidy/performance/ImplicitConversionInLoopCheck.h
  clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp
  clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.h
  clang-tools-extra/clang-tidy/plugin/ClangTidyPlugin.cpp
  clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.h
  clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h
  clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
  clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
  
clang-tools-extra/clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp
  clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp
  clang-tools-extra/clang-tidy/utils/ASTUtils.cpp
  clang-tools-extra/clang-tidy/utils/ASTUtils.h
  clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
  clang-tools-extra/clang-tidy/utils/IncludeInserter.cpp
  clang-tools-extra/clang-tidy/utils/IncludeSorter.cpp
  clang-tools-extra/clang-tidy/utils/IncludeSorter.h
  clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
  clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
  clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
  clang-tools-extra/clang-tidy/utils/TypeTraits.h

Index: clang-tools-extra/clang-tidy/utils/TypeTraits.h
===
--- clang-tools-extra/clang-tidy/utils/TypeTraits.h
+++ clang-tools-extra/clang-tidy/utils/TypeTraits.h
@@ -37,7 +37,7 @@
 /// Return true if `Type` has a non-trivial move assignment operator.
 bool hasNonTrivialMoveAssignm

[PATCH] D112864: [clang-tidy] Fix lint warnings in clang-tidy source code (NFC)

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

No functional change is intended.

The majority of clang-tidy check infractions were

- readability-identifier-naming
- llvm-qualified-auto
- llvm-namespace-comment

I have also fixed obvious typos wherever I noticed them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112864

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

[PATCH] D112864: [clang-tidy] Fix lint warnings in clang-tidy source code (NFC)

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

In D112864#3098351 , @carlosgalvezp 
wrote:

> Looks great, thanks for fixing! I'm surprised we don't have a CI bot running 
> these checks post-merge?

You would think so, but it looks like automatic checking during CI was disabled 
in this commit:
https://github.com/google/llvm-premerge-checks/commit/b69eb6f3647ecb67ff85e551323b3445acde684b

Anyway, even if you run clang-tidy manually, you could miss many issues. A 
number of warnings are in the header files, and they only get revealed if you 
set `--header-filter` properly to look at them.

I will leave this review open for a few days in case other people have comments 
to add. This patch isn't urgent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112864

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

[PATCH] D112864: [clang-tidy] Fix lint warnings in clang-tidy source code (NFC)

[Salman Javed via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGade0662c51b5: [clang-tidy] Fix lint warnings in clang-tidy 
source code (NFC) (authored by salman-javed-nz).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112864

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ClangTidy.h
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/clang-tidy/ClangTidyOptions.h
  clang-tools-extra/clang-tidy/abseil/DurationDivisionCheck.h
  clang-tools-extra/clang-tidy/abseil/DurationFactoryScaleCheck.cpp
  clang-tools-extra/clang-tidy/abseil/DurationRewriter.cpp
  clang-tools-extra/clang-tidy/abseil/DurationRewriter.h
  clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp
  clang-tools-extra/clang-tidy/altera/UnrollLoopsCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/CopyConstructorInitCheck.h
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/ForwardingReferenceOverloadCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/MultipleStatementMacroCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.h
  clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SuspiciousStringCompareCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnusedRaiiCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeMemberInitCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/SlicingCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/SlicingCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h
  clang-tools-extra/clang-tidy/fuchsia/MultipleInheritanceCheck.h
  clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
  clang-tools-extra/clang-tidy/misc/NoRecursionCheck.cpp
  clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
  clang-tools-extra/clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp
  clang-tools-extra/clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h
  clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
  clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
  clang-tools-extra/clang-tidy/modernize/ReplaceAutoPtrCheck.h
  
clang-tools-extra/clang-tidy/modernize/ReplaceDisallowCopyAndAssignMacroCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseNullptrCheck.h
  clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.h
  clang-tools-extra/clang-tidy/performance/ImplicitConversionInLoopCheck.cpp
  clang-tools-extra/clang-tidy/performance/ImplicitConversionInLoopCheck.h
  clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp
  clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.h
  clang-tools-extra/clang-tidy/plugin/ClangTidyPlugin.cpp
  clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.h
  clang-tools-extra/clang-tidy/readability/ContainerDataPointerCheck.h
  clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
  clang-tools-extra/clang-tidy/readability/FunctionCognitiveComplexityCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
  
clang-tools-extra/clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp
  clang-tools-extra/clang-tidy/readability/QualifiedAutoCheck.cpp
  clang-tools-extra/clang-tidy/utils/ASTUtils.cpp
  clang-tools-extra/clang-tidy/utils/ASTUtils.h
  clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
  clang-tools-extra/clang-tidy/utils/IncludeInserter.cpp
  clang-tools-extra/clang-tidy/utils/IncludeSorter.cpp
  clang-tools-extra/clang-tidy/utils/IncludeSorter.h
  clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
  clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
  clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
  clang-tools-extra/clang-tidy/utils/TypeTraits.h

Index: clang-tools-extra/clang-tidy/utils/TypeTraits.h
===
--- clang-tools-extra/clang-tidy/utils/TypeTraits.h
+++ clang-tools-extra/clang-tidy/utils/TypeTraits.h
@@ -37,7 +37,7 @@
 /// Return true if `Type` has a non-trivial move assignment operator.
 bool hasNonTrivialMoveAssignment(QualType Type);
 
-} // type_traits
+} // namespace type_traits
 } // namespace utils
 } // namespace tidy
 } // namespace clang
Index: clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
==

[PATCH] D112926: run-clang-tidy.py analyze unique files only

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

Why does the compile-commands.json have duplicate entries in the first place? 
Would someone do that on purpose?


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

https://reviews.llvm.org/D112926

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

[PATCH] D112926: run-clang-tidy.py analyze unique files only

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

It looks good to me. I don't have any more comments to add - it is a very small 
code change after all.
I have commit access and am happy to commit it on your behalf.

However, this would be my first time as a reviewer, and I don't want to be 
presumptuous and approve this before someone with more experience in the area 
has had a chance to look at it.

The rule of thumb is to wait a week, and if there's still no response, send a 
ping. Then we'll see what we can do to get this through.


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

https://reviews.llvm.org/D112926

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

[PATCH] D112926: run-clang-tidy.py analyze unique files only

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

In D112926#3113206 , @serkazi wrote:

> This is now accepted, please feel free to merge on my behalf. Thanks.

What's the full name and email you want associated with your commit?


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

https://reviews.llvm.org/D112926

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

[PATCH] D112926: run-clang-tidy.py analyze unique files only

[Salman Javed via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0dc856ed20e0: [clang-tidy] run-clang-tidy.py: analyze unique 
files only (authored by serkazi, committed by salman-javed-nz).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112926

Files:
  clang-tools-extra/clang-tidy/tool/run-clang-tidy.py


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -273,8 +273,8 @@
 
   # Load the database and extract all files.
   database = json.load(open(os.path.join(build_path, db_path)))
-  files = [make_absolute(entry['file'], entry['directory'])
-   for entry in database]
+  files = set([make_absolute(entry['file'], entry['directory'])
+   for entry in database])
 
   max_task = args.j
   if max_task == 0:


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -273,8 +273,8 @@
 
   # Load the database and extract all files.
   database = json.load(open(os.path.join(build_path, db_path)))
-  files = [make_absolute(entry['file'], entry['directory'])
-   for entry in database]
+  files = set([make_absolute(entry['file'], entry['directory'])
+   for entry in database])
 
   max_task = args.j
   if max_task == 0:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113450: [clang-tidy] Fix llvm-header-guard so that it works with Windows paths

[Salman Javed via Phabricator via cfe-commits]

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

Fixes https://bugs.llvm.org/show_bug.cgi?id=40372.

The llvm-header-guard check does not take into account that the path
separator on Windows is `\`, not `/`.

This means that instead of suggesting a header guard in the form of:
LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_FOO_H

it incorrectly suggests:
C:\LLVM_PROJECT\CLANG_TOOLS_EXTRA\CLANG_TIDY\FOO_H


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113450

Files:
  clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
  clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp


Index: clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
@@ -9,8 +9,6 @@
 namespace tidy {
 namespace test {
 
-// FIXME: It seems this might be incompatible to dos path. Investigating.
-#if !defined(_WIN32)
 static std::string runHeaderGuardCheck(StringRef Code, const Twine &Filename,
Optional ExpectedWarning) {
   std::vector Errors;
@@ -220,8 +218,38 @@
 runHeaderGuardCheck(
 "", "/llvm-project/clang-tools-extra/clangd/foo.h",
 StringRef("header is missing header guard")));
-}
+
+#ifdef WIN32
+  // Check interaction with Windows-style path separators (\).
+  EXPECT_EQ(
+  "#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+  "#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+  "\n"
+  "\n"
+  "#endif\n",
+  runHeaderGuardCheck("", "llvm-project\\clang-tools-extra\\clangd\\foo.h",
+  StringRef("header is missing header guard")));
+
+  EXPECT_EQ("#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+"#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+"\n"
+"\n"
+"#endif\n",
+runHeaderGuardCheck(
+"", "C:\\llvm-project\\clang-tools-extra\\clangd\\foo.h",
+StringRef("header is missing header guard")));
+
+  EXPECT_EQ(
+  "#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+  "#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+  "\n"
+  "\n"
+  "#endif\n",
+  runHeaderGuardCheck(
+  "", "SambaShare\\llvm-project\\clang-tools-extra\\clangd\\foo.h",
+  StringRef("header is missing header guard")));
 #endif
+}
 
 } // namespace test
 } // namespace tidy
Index: clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
===
--- clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
+++ clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
@@ -8,6 +8,7 @@
 
 #include "HeaderGuardCheck.h"
 #include "clang/Tooling/Tooling.h"
+#include "llvm/Support/Path.h"
 
 namespace clang {
 namespace tidy {
@@ -21,6 +22,10 @@
  StringRef OldGuard) {
   std::string Guard = tooling::getAbsolutePath(Filename);
 
+  // When running under Windows, need to convert the path separators from
+  // `\` to `/`.
+  Guard = llvm::sys::path::convert_to_slash(Guard);
+
   // Sanitize the path. There are some rules for compatibility with the 
historic
   // style in include/llvm and include/clang which we want to preserve.
 


Index: clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
@@ -9,8 +9,6 @@
 namespace tidy {
 namespace test {
 
-// FIXME: It seems this might be incompatible to dos path. Investigating.
-#if !defined(_WIN32)
 static std::string runHeaderGuardCheck(StringRef Code, const Twine &Filename,
Optional ExpectedWarning) {
   std::vector Errors;
@@ -220,8 +218,38 @@
 runHeaderGuardCheck(
 "", "/llvm-project/clang-tools-extra/clangd/foo.h",
 StringRef("header is missing header guard")));
-}
+
+#ifdef WIN32
+  // Check interaction with Windows-style path separators (\).
+  EXPECT_EQ(
+  "#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+  "#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+  "\n"
+  "\n"
+  "#endif\n",
+  runHeaderGuardCheck("", "llvm-project\\clang-tools-extra\\clangd\\foo.h",
+  StringRef("header is missing header guard")));
+
+  EXPECT_EQ("#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+"#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+"\n"
+"\n"
+"#endif\n",
+runHeaderGuardCheck(
+"

[PATCH] D113472: [clang-tidy] Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC)

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz created this revision.
salman-javed-nz added a reviewer: carlosgalvezp.
salman-javed-nz added a project: clang-tools-extra.
Herald added a subscriber: xazax.hun.
salman-javed-nz requested review of this revision.

Calling clang-tidy on ClangTidyDiagnosticConsumer.cpp gives a
"unmatched NOLINTBEGIN without a subsequent NOLINTEND" warning.

The "NOLINTBEGIN" and "NOLINTEND" string literals used in the
implementation of `createNolintError()` get mistaken for actual
NOLINTBEGIN/END comments used to suppress clang-tidy warnings.

Rewrite the string literals so that they can no longer be mistaken for
actual suppression comments.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113472

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp


Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -374,13 +374,11 @@
 bool IsNolintBegin) {
   ClangTidyError Error("clang-tidy-nolint", ClangTidyError::Error,
Context.getCurrentBuildDirectory(), false);
-  StringRef Message =
-  IsNolintBegin
-  ? "unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINTEND' "
-"comment"
-  : "unmatched 'NOLINTEND' comment without a previous 'NOLINTBEGIN' "
-"comment";
-  Error.Message = tooling::DiagnosticMessage(Message, SM, Loc);
+  auto Message = Twine("unmatched 'NOLINT") +
+ (IsNolintBegin ? "BEGIN" : "END") + "' comment without a " +
+ (IsNolintBegin ? "subsequent" : "previous") + " 'NOLINT" +
+ (IsNolintBegin ? "END" : "BEGIN") + "' comment";
+  Error.Message = tooling::DiagnosticMessage(Message.str(), SM, Loc);
   return Error;
 }
 


Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -374,13 +374,11 @@
 bool IsNolintBegin) {
   ClangTidyError Error("clang-tidy-nolint", ClangTidyError::Error,
Context.getCurrentBuildDirectory(), false);
-  StringRef Message =
-  IsNolintBegin
-  ? "unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINTEND' "
-"comment"
-  : "unmatched 'NOLINTEND' comment without a previous 'NOLINTBEGIN' "
-"comment";
-  Error.Message = tooling::DiagnosticMessage(Message, SM, Loc);
+  auto Message = Twine("unmatched 'NOLINT") +
+ (IsNolintBegin ? "BEGIN" : "END") + "' comment without a " +
+ (IsNolintBegin ? "subsequent" : "previous") + " 'NOLINT" +
+ (IsNolintBegin ? "END" : "BEGIN") + "' comment";
+  Error.Message = tooling::DiagnosticMessage(Message.str(), SM, Loc);
   return Error;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113472: [clang-tidy] Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC)

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

clang-tidy looking for `NOLINT` markers but not checking to see that the marker 
is within a comment, is long-standing behaviour at this point.
cpplint has the same behaviour, which explains why clang-tidy's implementation 
started off this way.

That's not to say that we have to keep this way forever. I would love for it to 
be improved.

However, discussing what the new behaviour should be and delivering it is 
beyond the scope of this patch, which is just trying to fix lint warnings in 
the current day framework.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113472

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

[PATCH] D113472: [clang-tidy] Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC)

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

I suspect that ensuring that `NOLINT`s are within a comment could be a 
reasonably large code change. If it is to be robust with `/* multiline comments 
*/`  and other shenanigans, then I would probably lean on the compiler to tell 
us what is and isn't a comment, instead of the simple text search approach we 
currently have.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113472

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

[PATCH] D113472: [clang-tidy] Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC)

[Salman Javed via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG00769572025f: [clang-tidy] Fix lint warning in 
ClangTidyDiagnosticConsumer.cpp (NFC) (authored by salman-javed-nz).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113472

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp


Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -374,13 +374,11 @@
 bool IsNolintBegin) {
   ClangTidyError Error("clang-tidy-nolint", ClangTidyError::Error,
Context.getCurrentBuildDirectory(), false);
-  StringRef Message =
-  IsNolintBegin
-  ? "unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINTEND' "
-"comment"
-  : "unmatched 'NOLINTEND' comment without a previous 'NOLINTBEGIN' "
-"comment";
-  Error.Message = tooling::DiagnosticMessage(Message, SM, Loc);
+  auto Message = Twine("unmatched 'NOLINT") +
+ (IsNolintBegin ? "BEGIN" : "END") + "' comment without a " +
+ (IsNolintBegin ? "subsequent" : "previous") + " 'NOLINT" +
+ (IsNolintBegin ? "END" : "BEGIN") + "' comment";
+  Error.Message = tooling::DiagnosticMessage(Message.str(), SM, Loc);
   return Error;
 }
 


Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -374,13 +374,11 @@
 bool IsNolintBegin) {
   ClangTidyError Error("clang-tidy-nolint", ClangTidyError::Error,
Context.getCurrentBuildDirectory(), false);
-  StringRef Message =
-  IsNolintBegin
-  ? "unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINTEND' "
-"comment"
-  : "unmatched 'NOLINTEND' comment without a previous 'NOLINTBEGIN' "
-"comment";
-  Error.Message = tooling::DiagnosticMessage(Message, SM, Loc);
+  auto Message = Twine("unmatched 'NOLINT") +
+ (IsNolintBegin ? "BEGIN" : "END") + "' comment without a " +
+ (IsNolintBegin ? "subsequent" : "previous") + " 'NOLINT" +
+ (IsNolintBegin ? "END" : "BEGIN") + "' comment";
+  Error.Message = tooling::DiagnosticMessage(Message.str(), SM, Loc);
   return Error;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113472: [clang-tidy] Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC)

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

Thanks for the review.

I will think some more about your suggestion to look for `//`. Once I have 
something that works, I will be back for another review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113472

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

[PATCH] D113450: [clang-tidy] Fix llvm-header-guard so that it works with Windows paths

[Salman Javed via Phabricator via cfe-commits]

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

Unit tests:

- Renamed Samba to SMB
- Added test for UNC paths


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113450

Files:
  clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
  clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp


Index: clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
@@ -9,8 +9,6 @@
 namespace tidy {
 namespace test {
 
-// FIXME: It seems this might be incompatible to dos path. Investigating.
-#if !defined(_WIN32)
 static std::string runHeaderGuardCheck(StringRef Code, const Twine &Filename,
Optional ExpectedWarning) {
   std::vector Errors;
@@ -220,8 +218,47 @@
 runHeaderGuardCheck(
 "", "/llvm-project/clang-tools-extra/clangd/foo.h",
 StringRef("header is missing header guard")));
-}
+
+#ifdef WIN32
+  // Check interaction with Windows-style path separators (\).
+  EXPECT_EQ(
+  "#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+  "#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+  "\n"
+  "\n"
+  "#endif\n",
+  runHeaderGuardCheck("", "llvm-project\\clang-tools-extra\\clangd\\foo.h",
+  StringRef("header is missing header guard")));
+
+  EXPECT_EQ("#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+"#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+"\n"
+"\n"
+"#endif\n",
+runHeaderGuardCheck(
+"", "C:\\llvm-project\\clang-tools-extra\\clangd\\foo.h",
+StringRef("header is missing header guard")));
+
+  EXPECT_EQ("#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+"#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+"\n"
+"\n"
+"#endif\n",
+runHeaderGuardCheck(
+"",
+"SMBShare\\llvm-project\\clang-tools-extra\\clangd\\foo.h",
+StringRef("header is missing header guard")));
+
+  EXPECT_EQ("#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+"#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+"\n"
+"\n"
+"#endif\n",
+runHeaderGuardCheck(
+"", 
"?\\C:\\llvm-project\\clang-tools-extra\\clangd\\foo.h",
+StringRef("header is missing header guard")));
 #endif
+}
 
 } // namespace test
 } // namespace tidy
Index: clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
===
--- clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
+++ clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
@@ -8,6 +8,7 @@
 
 #include "HeaderGuardCheck.h"
 #include "clang/Tooling/Tooling.h"
+#include "llvm/Support/Path.h"
 
 namespace clang {
 namespace tidy {
@@ -21,6 +22,10 @@
  StringRef OldGuard) {
   std::string Guard = tooling::getAbsolutePath(Filename);
 
+  // When running under Windows, need to convert the path separators from
+  // `\` to `/`.
+  Guard = llvm::sys::path::convert_to_slash(Guard);
+
   // Sanitize the path. There are some rules for compatibility with the 
historic
   // style in include/llvm and include/clang which we want to preserve.
 


Index: clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
@@ -9,8 +9,6 @@
 namespace tidy {
 namespace test {
 
-// FIXME: It seems this might be incompatible to dos path. Investigating.
-#if !defined(_WIN32)
 static std::string runHeaderGuardCheck(StringRef Code, const Twine &Filename,
Optional ExpectedWarning) {
   std::vector Errors;
@@ -220,8 +218,47 @@
 runHeaderGuardCheck(
 "", "/llvm-project/clang-tools-extra/clangd/foo.h",
 StringRef("header is missing header guard")));
-}
+
+#ifdef WIN32
+  // Check interaction with Windows-style path separators (\).
+  EXPECT_EQ(
+  "#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+  "#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+  "\n"
+  "\n"
+  "#endif\n",
+  runHeaderGuardCheck("", "llvm-project\\clang-tools-extra\\clangd\\foo.h",
+  StringRef("header is missing header guard")));
+
+  EXPECT_EQ("#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+"#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+"\n"
+"\n"
+"#endif\n",
+runHeaderGua

[PATCH] D113472: [clang-tidy] Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC)

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

In D113472#3119292 , @jgorbe wrote:

> This change is causing asan errors when running the clang-tidy checks under 
> ASan, most likely because of the reason akuegel pointed out in his comment. 
> I'm going to revert the patch to unbreak HEAD until the problem can be fixed.

If you could do that, that would be appreciated. Thank you. I won't be able to 
get back to my desk to revert myself for at least half an hour.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113472

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

[PATCH] D113472: [clang-tidy] Fix lint warning in ClangTidyDiagnosticConsumer.cpp (NFC)

[Salman Javed via Phabricator via cfe-commits]

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

Revert to using simple string literals. I was being too clever for my own good 
with the Twine.
I think this should no longer cause ASan issues. WDYT?

This was meant to be a simple patch, so sorry for all the trouble.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113472

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp


Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -376,10 +376,10 @@
Context.getCurrentBuildDirectory(), false);
   StringRef Message =
   IsNolintBegin
-  ? "unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINTEND' "
-"comment"
-  : "unmatched 'NOLINTEND' comment without a previous 'NOLINTBEGIN' "
-"comment";
+  ? ("unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINT"
+ "END' comment")
+  : ("unmatched 'NOLINTEND' comment without a previous 'NOLINT"
+ "BEGIN' comment");
   Error.Message = tooling::DiagnosticMessage(Message, SM, Loc);
   return Error;
 }


Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -376,10 +376,10 @@
Context.getCurrentBuildDirectory(), false);
   StringRef Message =
   IsNolintBegin
-  ? "unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINTEND' "
-"comment"
-  : "unmatched 'NOLINTEND' comment without a previous 'NOLINTBEGIN' "
-"comment";
+  ? ("unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINT"
+ "END' comment")
+  : ("unmatched 'NOLINTEND' comment without a previous 'NOLINT"
+ "BEGIN' comment");
   Error.Message = tooling::DiagnosticMessage(Message, SM, Loc);
   return Error;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113450: [clang-tidy] Fix llvm-header-guard so that it works with Windows paths

[Salman Javed via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb4f6f1c9369e: [clang-tidy] Fix llvm-header-guard so that it 
works with Windows paths (authored by salman-javed-nz).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113450

Files:
  clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
  clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp


Index: clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
@@ -9,8 +9,6 @@
 namespace tidy {
 namespace test {
 
-// FIXME: It seems this might be incompatible to dos path. Investigating.
-#if !defined(_WIN32)
 static std::string runHeaderGuardCheck(StringRef Code, const Twine &Filename,
Optional ExpectedWarning) {
   std::vector Errors;
@@ -220,8 +218,47 @@
 runHeaderGuardCheck(
 "", "/llvm-project/clang-tools-extra/clangd/foo.h",
 StringRef("header is missing header guard")));
-}
+
+#ifdef WIN32
+  // Check interaction with Windows-style path separators (\).
+  EXPECT_EQ(
+  "#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+  "#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+  "\n"
+  "\n"
+  "#endif\n",
+  runHeaderGuardCheck("", "llvm-project\\clang-tools-extra\\clangd\\foo.h",
+  StringRef("header is missing header guard")));
+
+  EXPECT_EQ("#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+"#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+"\n"
+"\n"
+"#endif\n",
+runHeaderGuardCheck(
+"", "C:\\llvm-project\\clang-tools-extra\\clangd\\foo.h",
+StringRef("header is missing header guard")));
+
+  EXPECT_EQ("#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+"#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+"\n"
+"\n"
+"#endif\n",
+runHeaderGuardCheck(
+"",
+"SMBShare\\llvm-project\\clang-tools-extra\\clangd\\foo.h",
+StringRef("header is missing header guard")));
+
+  EXPECT_EQ("#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+"#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+"\n"
+"\n"
+"#endif\n",
+runHeaderGuardCheck(
+"", 
"?\\C:\\llvm-project\\clang-tools-extra\\clangd\\foo.h",
+StringRef("header is missing header guard")));
 #endif
+}
 
 } // namespace test
 } // namespace tidy
Index: clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
===
--- clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
+++ clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
@@ -8,6 +8,7 @@
 
 #include "HeaderGuardCheck.h"
 #include "clang/Tooling/Tooling.h"
+#include "llvm/Support/Path.h"
 
 namespace clang {
 namespace tidy {
@@ -21,6 +22,10 @@
  StringRef OldGuard) {
   std::string Guard = tooling::getAbsolutePath(Filename);
 
+  // When running under Windows, need to convert the path separators from
+  // `\` to `/`.
+  Guard = llvm::sys::path::convert_to_slash(Guard);
+
   // Sanitize the path. There are some rules for compatibility with the 
historic
   // style in include/llvm and include/clang which we want to preserve.
 


Index: clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
@@ -9,8 +9,6 @@
 namespace tidy {
 namespace test {
 
-// FIXME: It seems this might be incompatible to dos path. Investigating.
-#if !defined(_WIN32)
 static std::string runHeaderGuardCheck(StringRef Code, const Twine &Filename,
Optional ExpectedWarning) {
   std::vector Errors;
@@ -220,8 +218,47 @@
 runHeaderGuardCheck(
 "", "/llvm-project/clang-tools-extra/clangd/foo.h",
 StringRef("header is missing header guard")));
-}
+
+#ifdef WIN32
+  // Check interaction with Windows-style path separators (\).
+  EXPECT_EQ(
+  "#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+  "#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+  "\n"
+  "\n"
+  "#endif\n",
+  runHeaderGuardCheck("", "llvm-project\\clang-tools-extra\\clangd\\foo.h",
+  StringRef("header is missing header guard")));
+
+  EXPECT_EQ("#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+"#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_FOO_H\n"
+"\n"
+ 

[PATCH] D113450: [clang-tidy] Fix llvm-header-guard so that it works with Windows paths

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

@aaron.ballman -
I've added the unit test for UNC path as you suggested. Since you've already 
given the LGTM, I assume you don't need to see the patch again, so I have gone 
ahead with the commit.
Anyway, I'll be around to address any problems if they crop up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113450

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

[PATCH] D113830: [clang-tidy] Fix false positive in `readability-identifier-naming` check involving `override` attribute

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

Happy to take a look at this, and do some of the initial review legwork, but 
let's leave final approval to @aaron.ballman.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113830

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

[PATCH] D113830: [clang-tidy] Fix false positive in `readability-identifier-naming` check involving `override` attribute

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:1260
 if (Decl->isMain() || !Decl->isUserProvided() ||
-Decl->size_overridden_methods() > 0)
+Decl->size_overridden_methods() > 0 || Decl->hasAttr())
   return SK_Invalid;

Adding a call to `hasAttr()` looks OK to me -- this is in line 
with [[ https://clang.llvm.org/doxygen/ASTMatchers_8h_source.html#l06002 | 
AST_MATCHER(CXXMethodDecl, isOverride)]]. Other clang-tidy checks have done the 
same thing.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp:282-294
 class AOverridden {
 public:
   virtual ~AOverridden() = default;
   virtual void BadBaseMethod() = 0;
   // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for virtual 
method 'BadBaseMethod'
   // CHECK-FIXES: {{^}}  virtual void v_Bad_Base_Method() = 0;
 };

How about a test to check that diagnostics are generated even if the `override` 
keyword is omitted.
This challenges the `Decl->size_overridden_methods() > 0` portion of the `if` 
statement.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp:321
+  //(note that there could be specializations of the template base 
class, though)
+  void BadBaseMethod() override{}
+};

What should happen if the base method is overridden without the `override` 
keyword?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113830

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

[PATCH] D113830: [clang-tidy] Fix false positive in `readability-identifier-naming` check involving `override` attribute

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp:329
+
+  // FIXME: The fixes from ATOverridden should be propagated to the following 
call
+  a_vTitem.BadBaseMethod();

The fixes /do/ seem to be propagated if there is an explicit template 
instantiation.

```lang=cpp
template 
struct A
{
virtual void method() {}
};

template 
struct B : A
{
void method() override {}
void CallVirtual() { this->method(); }
};

template class B;
```

Running with `clang-tidy -fix` and `readability-identifier-naming.FunctionCase 
= CamelCase` gives:
```lang=cpp
template 
struct A
{
virtual void Method() {}
};

template 
struct B : A
{
void Method() override {}
void CallVirtual() { this->Method(); }
};

template class B;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113830

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

[PATCH] D113847: [clang-tidy][NFC] Simplify ClangTidyStats

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:50
 struct ClangTidyStats {
-  ClangTidyStats()
-  : ErrorsDisplayed(0), ErrorsIgnoredCheckFilter(0), 
ErrorsIgnoredNOLINT(0),
-ErrorsIgnoredNonUserCode(0), ErrorsIgnoredLineFilter(0) {}
+  int ErrorsDisplayed{0};
+  int ErrorsIgnoredCheckFilter{0};

What's the prevalent style for class member initialization? `=` or `{}`?
cppcoreguidelines-prefer-member-initializer defaults to `{}` but I have seen 
both types in the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113847

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

[PATCH] D113830: [clang-tidy] Fix false positive in `readability-identifier-naming` check involving `override` attribute

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp:299
   // CHECK-FIXES: {{^}}  void v_Bad_Base_Method() override {}
+  void BadBaseMethodNoAttr() {}
+  // CHECK-FIXES: {{^}}  void v_Bad_Base_Method_No_Attr() {}

I think this line could spell out more clearly that it is testing the case 
where the `override` keyword is omitted. I don't think the `NoAttr()` suffix 
says enough. `override` is not the only Attr.

Possible solutions:
- You could incorporate the word "override" in the method name e.g. 
`BadBaseMethodNoOverride()`, or
- You could put a `/* override */` where `override` normally would be to bring 
attention to the fact that it is missing, or
- You could add a comment explaining what's going on, like the `// Overriding a 
badly-named base isn't a new violation` a couple of lines above.

Feel free to do what you think is the least janky.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp:299
   // CHECK-FIXES: {{^}}  void v_Bad_Base_Method() override {}
+  void BadBaseMethodNoAttr() {}
+  // CHECK-FIXES: {{^}}  void v_Bad_Base_Method_No_Attr() {}

salman-javed-nz wrote:
> I think this line could spell out more clearly that it is testing the case 
> where the `override` keyword is omitted. I don't think the `NoAttr()` suffix 
> says enough. `override` is not the only Attr.
> 
> Possible solutions:
> - You could incorporate the word "override" in the method name e.g. 
> `BadBaseMethodNoOverride()`, or
> - You could put a `/* override */` where `override` normally would be to 
> bring attention to the fact that it is missing, or
> - You could add a comment explaining what's going on, like the `// Overriding 
> a badly-named base isn't a new violation` a couple of lines above.
> 
> Feel free to do what you think is the least janky.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp:313
+BadBaseMethodNoAttr();
+// CHECK-FIXES: {{^}}v_Bad_Base_Method_No_Attr();
   }

I would throw in the
```
this->BadBaseMethodNoAttr();
AOverridden::BadBaseMethodNoAttr();
COverriding::BadBaseMethodNoAttr();
```
lines as well.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp:335
+  //(note that there could be specializations of the template base 
class, though)
+  void BadBaseMethod() override{}
+





Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp:365
+  // Overriding a badly-named base isn't a new violation.
+  void BadBaseMethod() override{}
+  // CHECK-FIXES: {{^}}  void v_Bad_Base_Method() override{}





Comment at: 
clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp:370
+template class CTIOverriding;
+
 template 

Are you missing another `VirtualCall()` function here? (to test `ATIOverridden` 
and `CTIOverriding`?)


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

https://reviews.llvm.org/D113830

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

[PATCH] D113830: [clang-tidy] Fix false positive in `readability-identifier-naming` check involving `override` attribute

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz accepted this revision.
salman-javed-nz added a comment.
This revision is now accepted and ready to land.

LGTM. Nothing more to suggest from my side. Can we allow a few days for the 
other reviewers to put in their 2c.

As for the Bugzilla ticket , I'd 
say you've done enough to satisfy the primary grievance expressed in the PR: 
warnings should be raised in the base class only.
This is in line with the check's state purpose in the documentation 
.

> The naming of virtual methods is reported where they occur in the base class, 
> but not where they are overridden, as it can’t be fixed locally there.

The comments below the PR talk about the need to propagate naming fixes to the 
derived classes and call sites. The last comment shows an example where fixes 
are not propagated at all, not even for the simple (non-template) case. But 
your tests for 
`COverriding` and `CTIOverriding` show that the replacements are indeed 
working. In any case, applying fixes outside the base class is going above and 
beyond the check's scope, and as you say, could lead to incorrect suggestions 
if the code is even slightly less trivial. More work could be done on this 
class, but doesn't have to be in this patch.


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

https://reviews.llvm.org/D113830

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

[PATCH] D114149: [clang-tidy] Fix pr48613: "llvm-header-guard uses a reserved identifier"

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz created this revision.
salman-javed-nz added reviewers: whisperity, hokein, aaron.ballman.
salman-javed-nz added a project: clang-tools-extra.
Herald added subscribers: carlosgalvezp, rnkovacs, xazax.hun.
salman-javed-nz requested review of this revision.

Fixes https://bugs.llvm.org/show_bug.cgi?id=48613.

llvm-header-guard is suggesting header guards with leading underscores
if the header file path begins with a '/' or similar special character.
Only reserved identifiers should begin with an underscore.

Another problem is that llvm-header-guard does the character
substitution for '/', '.', and '-' only.
There are other characters which are valid in a file path but invalid
in a macro name. Rely on the functions provided in CharInfo.h to
identify those characters.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114149

Files:
  clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp
  clang-tools-extra/clang-tidy/utils/HeaderGuard.h
  clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp

Index: clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
@@ -219,6 +219,25 @@
 "", "/llvm-project/clang-tools-extra/clangd/foo.h",
 StringRef("header is missing header guard")));
 
+  // Characters outside [A-z0-9] are replaced with "_".
+  EXPECT_EQ("#ifndef FOO_BAR_TE_T_A_B_C_H\n"
+"#define FOO_BAR_TE_T_A_B_C_H\n"
+"\n"
+"\n"
+"#endif\n",
+runHeaderGuardCheck("", "include/foo-bar/te$t/a(b)c.h",
+StringRef("header is missing header guard")));
+
+  // Subtitution of characters should not result in a header guard starting
+  // with "_".
+  EXPECT_EQ("#ifndef BAR_H\n"
+"#define BAR_H\n"
+"\n"
+"\n"
+"#endif\n",
+runHeaderGuardCheck("", "include/$bar.h",
+StringRef("header is missing header guard")));
+
 #ifdef WIN32
   // Check interaction with Windows-style path separators (\).
   EXPECT_EQ(
Index: clang-tools-extra/clang-tidy/utils/HeaderGuard.h
===
--- clang-tools-extra/clang-tidy/utils/HeaderGuard.h
+++ clang-tools-extra/clang-tidy/utils/HeaderGuard.h
@@ -38,6 +38,9 @@
   void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override;
 
+  /// Ensure that the provided header guard is a valid identifier.
+  static std::string sanitizeHeaderGuard(StringRef Guard);
+
   /// Returns ``true`` if the check should suggest inserting a trailing comment
   /// on the ``#endif`` of the header guard. It will use the same name as
   /// returned by ``HeaderGuardCheck::getHeaderGuard``.
Index: clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp
===
--- clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp
+++ clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp
@@ -24,6 +24,27 @@
   return std::string(Result.str());
 }
 
+/// Replace invalid characters in the identifier with '_'.
+static std::string replaceInvalidChars(StringRef Identifier) {
+  std::string Fixed{Identifier};
+  for (auto It = Fixed.begin(); It != Fixed.end(); ++It) {
+// Identifier name must start with [A-z].
+if (It == Fixed.begin() && isAsciiIdentifierStart(*It))
+  continue;
+// The subsequent characters can be [A-z0-9].
+if (It > Fixed.begin() && isAsciiIdentifierContinue(*It))
+  continue;
+*It = '_';
+  }
+  return Fixed;
+}
+
+/// Remove all '_' characters at the beginning of the identifier. Only reserved
+/// identifiers are allowed to start with these.
+static StringRef dropLeadingUnderscores(StringRef Identifier) {
+  return Identifier.drop_while([](char c) { return c == '_'; });
+}
+
 namespace {
 class HeaderGuardPPCallbacks : public PPCallbacks {
 public:
@@ -164,6 +185,7 @@
  StringRef CurHeaderGuard,
  std::vector &FixIts) {
 std::string CPPVar = Check->getHeaderGuard(FileName, CurHeaderGuard);
+CPPVar = HeaderGuardCheck::sanitizeHeaderGuard(CPPVar);
 std::string CPPVarUnder = CPPVar + '_';
 
 // Allow a trailing underscore iff we don't have to change the endif comment
@@ -216,6 +238,7 @@
 continue;
 
   std::string CPPVar = Check->getHeaderGuard(FileName);
+  CPPVar = HeaderGuardCheck::sanitizeHeaderGuard(CPPVar);
   std::string CPPVarUnder = CPPVar + '_'; // Allow a trailing underscore.
   // If there's a macro with a name that follows the header guard convention
   // but was not recognized by the preprocessor as a header guard there must
@@ -278,6 +301,11 @@
   P

[PATCH] D114149: [clang-tidy] Fix pr48613: "llvm-header-guard uses a reserved identifier"

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp:28
+/// Replace invalid characters in the identifier with '_'.
+static std::string replaceInvalidChars(StringRef Identifier) {
+  std::string Fixed{Identifier};

aaron.ballman wrote:
> This fixes some issues but I think it introduces new issues too. The old code 
> would accept Unicode characters and pass them along as (valid) identifier 
> characters. The new code will replace all the Unicode characters with `_` 
> which means some people's header guards may go from useful to 
> `___`. We should definitely add test cases for Unicode file names.
> 
> Ultimately, I think this functionality will want to use `UnicodeCharSets.h` 
> to implement something along the lines of what's done in 
> `isAllowedInitiallyIDChar()` and `isAllowedIDChar()` from Lexer.cpp.
`isAllowed*IDChar()`does exactly what I want, but is a static function in 
Lexer.cpp.
Should I be changing its declaration so that it's exposed via Lexer.h?
I don't know what the bar is to warrant changing core public Clang API. 

If I can just call the function directly from llvm-header-guard, I can avoid 
code duplication.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114149

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

[PATCH] D114149: [clang-tidy] Fix pr48613: "llvm-header-guard uses a reserved identifier"

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz marked an inline comment as not done.
salman-javed-nz added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp:28
+/// Replace invalid characters in the identifier with '_'.
+static std::string replaceInvalidChars(StringRef Identifier) {
+  std::string Fixed{Identifier};

aaron.ballman wrote:
> salman-javed-nz wrote:
> > aaron.ballman wrote:
> > > This fixes some issues but I think it introduces new issues too. The old 
> > > code would accept Unicode characters and pass them along as (valid) 
> > > identifier characters. The new code will replace all the Unicode 
> > > characters with `_` which means some people's header guards may go from 
> > > useful to `___`. We should definitely add test cases for 
> > > Unicode file names.
> > > 
> > > Ultimately, I think this functionality will want to use 
> > > `UnicodeCharSets.h` to implement something along the lines of what's done 
> > > in `isAllowedInitiallyIDChar()` and `isAllowedIDChar()` from Lexer.cpp.
> > `isAllowed*IDChar()`does exactly what I want, but is a static function in 
> > Lexer.cpp.
> > Should I be changing its declaration so that it's exposed via Lexer.h?
> > I don't know what the bar is to warrant changing core public Clang API. 
> > 
> > If I can just call the function directly from llvm-header-guard, I can 
> > avoid code duplication.
> > 
> > 
> We could expose the interface from `Lexer` if we feel that's the right 
> approach. I was going back and forth on whether it was the right approach and 
> eventually thought that we don't want to expose it directly because I don't 
> think we want clang-tidy to care about things like `LangOpts.AsmPreprocessor` 
> or even `LangOpts.DollarIdents`.
> 
> So my recommendation is to put this into LexerUtils.h/.cpp in clang-tidy 
> rather than changing the Clang Lexer interface. WDYT?
//(For brevity, when I say `isAllowedIDChar`, I mean both `isAllowedIDChar` and 
`isAllowedInitiallyIDChar`)//

I see two possible directions:

If I expose the interface from Lexer.h, then in Clang-Tidy, it's only a matter 
of calling 
`isAllowedIDChar(C, Context->getLangOpts())`. Clang-Tidy doesn't need to care 
about the contents of `LangOpts` - it just needs to pass them on as-is to the 
`isAllowedIDChar()` function.
`getLangOpts()` is already used all over the Clang-Tidy code base.

The other way is to create a copy of `isAllowedIDChar()` in the Clang-Tidy 
project and take out the parts to do with `LangOpts.AsmPreprocessor` and 
`LangOpts.DollarIdents`.  Now we have an implementation tailored to just the 
things Clang-Tidy cares about. I will need to add `clangLex` to CMakeLists.txt 
to gain visibility of clang/lib/Lex/UnicodeCharSets.h's character tables.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114149

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

[PATCH] D114149: [clang-tidy] Fix pr48613: "llvm-header-guard uses a reserved identifier"

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz marked an inline comment as not done.
salman-javed-nz added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp:28
+/// Replace invalid characters in the identifier with '_'.
+static std::string replaceInvalidChars(StringRef Identifier) {
+  std::string Fixed{Identifier};

salman-javed-nz wrote:
> aaron.ballman wrote:
> > salman-javed-nz wrote:
> > > aaron.ballman wrote:
> > > > This fixes some issues but I think it introduces new issues too. The 
> > > > old code would accept Unicode characters and pass them along as (valid) 
> > > > identifier characters. The new code will replace all the Unicode 
> > > > characters with `_` which means some people's header guards may go from 
> > > > useful to `___`. We should definitely add test cases for 
> > > > Unicode file names.
> > > > 
> > > > Ultimately, I think this functionality will want to use 
> > > > `UnicodeCharSets.h` to implement something along the lines of what's 
> > > > done in `isAllowedInitiallyIDChar()` and `isAllowedIDChar()` from 
> > > > Lexer.cpp.
> > > `isAllowed*IDChar()`does exactly what I want, but is a static function in 
> > > Lexer.cpp.
> > > Should I be changing its declaration so that it's exposed via Lexer.h?
> > > I don't know what the bar is to warrant changing core public Clang API. 
> > > 
> > > If I can just call the function directly from llvm-header-guard, I can 
> > > avoid code duplication.
> > > 
> > > 
> > We could expose the interface from `Lexer` if we feel that's the right 
> > approach. I was going back and forth on whether it was the right approach 
> > and eventually thought that we don't want to expose it directly because I 
> > don't think we want clang-tidy to care about things like 
> > `LangOpts.AsmPreprocessor` or even `LangOpts.DollarIdents`.
> > 
> > So my recommendation is to put this into LexerUtils.h/.cpp in clang-tidy 
> > rather than changing the Clang Lexer interface. WDYT?
> //(For brevity, when I say `isAllowedIDChar`, I mean both `isAllowedIDChar` 
> and `isAllowedInitiallyIDChar`)//
> 
> I see two possible directions:
> 
> If I expose the interface from Lexer.h, then in Clang-Tidy, it's only a 
> matter of calling 
> `isAllowedIDChar(C, Context->getLangOpts())`. Clang-Tidy doesn't need to care 
> about the contents of `LangOpts` - it just needs to pass them on as-is to the 
> `isAllowedIDChar()` function.
> `getLangOpts()` is already used all over the Clang-Tidy code base.
> 
> The other way is to create a copy of `isAllowedIDChar()` in the Clang-Tidy 
> project and take out the parts to do with `LangOpts.AsmPreprocessor` and 
> `LangOpts.DollarIdents`.  Now we have an implementation tailored to just the 
> things Clang-Tidy cares about. I will need to add `clangLex` to 
> CMakeLists.txt to gain visibility of clang/lib/Lex/UnicodeCharSets.h's 
> character tables.
I need to make a correction to my previous comment (I swear Phabricator had an 
"edit comment" button but I just don't see it at the moment).

> The other way is to create a copy of `isAllowedIDChar()` in the Clang-Tidy 
> project and take out the parts to do with `LangOpts.AsmPreprocessor` and 
> `LangOpts.DollarIdents`.  Now we have an implementation tailored to just the 
> things Clang-Tidy cares about. I will need to add `clangLex` to 
> CMakeLists.txt to gain visibility of clang/lib/Lex/UnicodeCharSets.h's 
> character tables.

Of course clangTidyUtils already depends on clangLex, so we don't need another 
`add_clang_library` line in CMakeLists.txt, contrary to what I said earlier. 
However, UnicodeCharSets.h is still doesn't come up in Clang-Tidy's header 
search path because it doesn't sit in the include/clang folder. The file will 
need moving.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114149

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

[PATCH] D113830: [clang-tidy] Fix false positive in `readability-identifier-naming` check involving `override` attribute

[Salman Javed via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG625901636134: [clang-tidy] Fix false positive in 
readability-identifier-naming check… (authored by fwolff, committed by 
salman-javed-nz).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113830

Files:
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming.cpp
@@ -285,6 +285,10 @@
   virtual void BadBaseMethod() = 0;
   // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for virtual method 'BadBaseMethod'
   // CHECK-FIXES: {{^}}  virtual void v_Bad_Base_Method() = 0;
+
+  virtual void BadBaseMethodNoAttr() = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for virtual method 'BadBaseMethodNoAttr'
+  // CHECK-FIXES: {{^}}  virtual void v_Bad_Base_Method_No_Attr() = 0;
 };
 
 class COverriding : public AOverridden {
@@ -293,6 +297,9 @@
   void BadBaseMethod() override {}
   // CHECK-FIXES: {{^}}  void v_Bad_Base_Method() override {}
 
+  void BadBaseMethodNoAttr() /* override */ {}
+  // CHECK-FIXES: {{^}}  void v_Bad_Base_Method_No_Attr() /* override */ {}
+
   void foo() {
 BadBaseMethod();
 // CHECK-FIXES: {{^}}v_Bad_Base_Method();
@@ -302,12 +309,79 @@
 // CHECK-FIXES: {{^}}AOverridden::v_Bad_Base_Method();
 COverriding::BadBaseMethod();
 // CHECK-FIXES: {{^}}COverriding::v_Bad_Base_Method();
+
+BadBaseMethodNoAttr();
+// CHECK-FIXES: {{^}}v_Bad_Base_Method_No_Attr();
+this->BadBaseMethodNoAttr();
+// CHECK-FIXES: {{^}}this->v_Bad_Base_Method_No_Attr();
+AOverridden::BadBaseMethodNoAttr();
+// CHECK-FIXES: {{^}}AOverridden::v_Bad_Base_Method_No_Attr();
+COverriding::BadBaseMethodNoAttr();
+// CHECK-FIXES: {{^}}COverriding::v_Bad_Base_Method_No_Attr();
   }
 };
 
-void VirtualCall(AOverridden &a_vItem) {
+// Same test as above, now with a dependent base class.
+template
+class ATOverridden {
+public:
+  virtual void BadBaseMethod() = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for virtual method 'BadBaseMethod'
+  // CHECK-FIXES: {{^}}  virtual void v_Bad_Base_Method() = 0;
+
+  virtual void BadBaseMethodNoAttr() = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for virtual method 'BadBaseMethodNoAttr'
+  // CHECK-FIXES: {{^}}  virtual void v_Bad_Base_Method_No_Attr() = 0;
+};
+
+template
+class CTOverriding : public ATOverridden {
+  // Overriding a badly-named base isn't a new violation.
+  // FIXME: The fixes from the base class should be propagated to the derived class here
+  //(note that there could be specializations of the template base class, though)
+  void BadBaseMethod() override {}
+
+  // Without the "override" attribute, and due to the dependent base class, it is not
+  // known whether this method overrides anything, so we get the warning here.
+  virtual void BadBaseMethodNoAttr() {};
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for virtual method 'BadBaseMethodNoAttr'
+  // CHECK-FIXES: {{^}}  virtual void v_Bad_Base_Method_No_Attr() {};
+};
+
+template
+void VirtualCall(AOverridden &a_vItem, ATOverridden &a_vTitem) {
+  a_vItem.BadBaseMethod();
+  // CHECK-FIXES: {{^}}  a_vItem.v_Bad_Base_Method();
+
+  // FIXME: The fixes from ATOverridden should be propagated to the following call
+  a_vTitem.BadBaseMethod();
+}
+
+// Same test as above, now with a dependent base class that is instantiated below.
+template
+class ATIOverridden {
+public:
+  virtual void BadBaseMethod() = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: invalid case style for virtual method 'BadBaseMethod'
+  // CHECK-FIXES: {{^}}  virtual void v_Bad_Base_Method() = 0;
+};
+
+template
+class CTIOverriding : public ATIOverridden {
+public:
+  // Overriding a badly-named base isn't a new violation.
+  void BadBaseMethod() override {}
+  // CHECK-FIXES: {{^}}  void v_Bad_Base_Method() override {}
+};
+
+template class CTIOverriding;
+
+void VirtualCallI(ATIOverridden& a_vItem, CTIOverriding& a_vCitem) {
   a_vItem.BadBaseMethod();
   // CHECK-FIXES: {{^}}  a_vItem.v_Bad_Base_Method();
+
+  a_vCitem.BadBaseMethod();
+  // CHECK-FIXES: {{^}}  a_vCitem.v_Bad_Base_Method();
 }
 
 template 
Index: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck

[PATCH] D114149: [clang-tidy] Fix pr48613: "llvm-header-guard uses a reserved identifier"

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz updated this revision to Diff 388699.
salman-javed-nz edited the summary of this revision.
salman-javed-nz added a comment.

Back out the "replace invalid characters in an identifier with underscores" 
feature.
Making this feature work with Unicode characters on different operating systems 
significantly increases the amount of code change required.
This can always be revisited in another patch.

In this patch, let's focus on fixing the most pressing problem at hand first: 
header guards starting with underscores.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114149

Files:
  clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp
  clang-tools-extra/clang-tidy/utils/HeaderGuard.h
  clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp


Index: clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
@@ -219,6 +219,16 @@
 "", "/llvm-project/clang-tools-extra/clangd/foo.h",
 StringRef("header is missing header guard")));
 
+  // Substitution of characters should not result in a header guard starting
+  // with "_".
+  EXPECT_EQ("#ifndef BAR_H\n"
+"#define BAR_H\n"
+"\n"
+"\n"
+"#endif\n",
+runHeaderGuardCheck("", "include/--bar.h",
+StringRef("header is missing header guard")));
+
 #ifdef WIN32
   // Check interaction with Windows-style path separators (\).
   EXPECT_EQ(
Index: clang-tools-extra/clang-tidy/utils/HeaderGuard.h
===
--- clang-tools-extra/clang-tidy/utils/HeaderGuard.h
+++ clang-tools-extra/clang-tidy/utils/HeaderGuard.h
@@ -38,6 +38,9 @@
   void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override;
 
+  /// Ensure that the provided header guard is a non-reserved identifier.
+  std::string sanitizeHeaderGuard(StringRef Guard);
+
   /// Returns ``true`` if the check should suggest inserting a trailing comment
   /// on the ``#endif`` of the header guard. It will use the same name as
   /// returned by ``HeaderGuardCheck::getHeaderGuard``.
Index: clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp
===
--- clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp
+++ clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp
@@ -164,9 +164,10 @@
  StringRef CurHeaderGuard,
  std::vector &FixIts) {
 std::string CPPVar = Check->getHeaderGuard(FileName, CurHeaderGuard);
+CPPVar = Check->sanitizeHeaderGuard(CPPVar);
 std::string CPPVarUnder = CPPVar + '_';
 
-// Allow a trailing underscore iff we don't have to change the endif 
comment
+// Allow a trailing underscore if we don't have to change the endif comment
 // too.
 if (Ifndef.isValid() && CurHeaderGuard != CPPVar &&
 (CurHeaderGuard != CPPVarUnder ||
@@ -216,6 +217,7 @@
 continue;
 
   std::string CPPVar = Check->getHeaderGuard(FileName);
+  CPPVar = Check->sanitizeHeaderGuard(CPPVar);
   std::string CPPVarUnder = CPPVar + '_'; // Allow a trailing underscore.
   // If there's a macro with a name that follows the header guard 
convention
   // but was not recognized by the preprocessor as a header guard there 
must
@@ -278,6 +280,11 @@
   PP->addPPCallbacks(std::make_unique(PP, this));
 }
 
+std::string HeaderGuardCheck::sanitizeHeaderGuard(StringRef Guard) {
+  // Only reserved identifiers are allowed to start with an '_'.
+  return Guard.drop_while([](char C) { return C == '_'; }).str();
+}
+
 bool HeaderGuardCheck::shouldSuggestEndifComment(StringRef FileName) {
   return utils::isFileExtension(FileName, HeaderFileExtensions);
 }


Index: clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
@@ -219,6 +219,16 @@
 "", "/llvm-project/clang-tools-extra/clangd/foo.h",
 StringRef("header is missing header guard")));
 
+  // Substitution of characters should not result in a header guard starting
+  // with "_".
+  EXPECT_EQ("#ifndef BAR_H\n"
+"#define BAR_H\n"
+"\n"
+"\n"
+"#endif\n",
+runHeaderGuardCheck("", "include/--bar.h",
+StringRef("header is missing header guard")));
+
 #ifdef WIN32
   // Check interaction with Windows-style path separators (\).
   EXPECT_EQ(
Index: clang-tools-extra/cla

[PATCH] D114149: [clang-tidy] Fix pr48613: "llvm-header-guard uses a reserved identifier"

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz marked 4 inline comments as done.
salman-javed-nz added a comment.

I reverted my changes to do with the invalid character substitution. Doing 
something akin to `isAllowedInitiallyIDChar()` and `isAllowedIDChar()` in 
Lexer.cpp will require converting from `char*` to `UTF32*`. Windows complicates 
things by using its own code page (typically Windows-1252). Will require a lot 
of careful consideration to implement correctly.

Changing this was never part of the PR's scope, so I have reverted it for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114149

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

[PATCH] D114317: [clang-tidy][WIP] Do not run perfect alias checks

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

What would you say are the key differences between this patch differ and 
previous attempts, e.g. https://reviews.llvm.org/D72566? How does this patch 
address the concerns raised in the previous reviews?


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

https://reviews.llvm.org/D114317

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

Thanks for the patch.

Just a little bit of feedback but overall I'm happy with the approach taken.




Comment at: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:99-106
+  CharSourceRange ReplaceRange;
+  if (isa(CastExpr))
+ReplaceRange = CharSourceRange::getCharRange(
+CastExpr->getLParenLoc(),
+CastExpr->getSubExprAsWritten()->getBeginLoc());
+  else if (isa(CastExpr))
+ReplaceRange = CharSourceRange::getCharRange(CastExpr->getBeginLoc(),

The majority of `checkExpr()`'s contents are common to both types, 
`CStyleCastExpr` and `CXXFunctionalCastExpr`.
Only the `ReplaceRange = CharSourceRange::getCharRange...` and the 
`DestTypeString = Lexer::getSourceText...` parts change depending on the Expr 
type.

What about breaking those two assignments out into their own functions, rather 
than templating the entire `checkExpr()` function?

For example, (note: untested code)

```lang=cpp
clang::CharSourceRange GetReplaceRange(const CStyleCastExpr *CastExpr) {
  return CharSourceRange::getCharRange(
  CastExpr->getLParenLoc(), CastExpr->getSubExprAsWritten()->getBeginLoc());
}

clang::CharSourceRange GetReplaceRange(const CXXFunctionalCastExpr *CastExpr) {
  return CharSourceRange::getCharRange(CastExpr->getBeginLoc(),
   CastExpr->getLParenLoc());
}

...

CharSourceRange ReplaceRange =
  isa(CastExpr)
  ? GetReplaceRange(dyn_cast(CastExpr))
  : GetReplaceRange(dyn_cast(CastExpr));
```

Would something like that work?



Comment at: 
clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:151-162
+  if (isa(CastExpr))
+DestTypeString =
+Lexer::getSourceText(CharSourceRange::getTokenRange(
+ CastExpr->getLParenLoc().getLocWithOffset(1),
+ 
CastExpr->getRParenLoc().getLocWithOffset(-1)),
+ SM, getLangOpts());
+  else if (isa(CastExpr))

See comment above.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:139-140
 
+- Updated `google-readability-casting` to diagnose and fix functional casts, 
to achieve feature
+  parity with the corresponding `cpplint.py` check.
 

Single back-ticks are used to do linking. Double back-ticks is probably what 
you're after.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:335
+  const char *str = "foo";
+  auto s = S(str);
+}

Is a test to check `new int(x)` worth including? I see that the cpplint guys 
explicitly filter it out of their results.


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:100-104
+  if (isa(CastExpr))
+ReplaceRange = CharSourceRange::getCharRange(
+CastExpr->getLParenLoc(),
+CastExpr->getSubExprAsWritten()->getBeginLoc());
+  else if (isa(CastExpr))

Should this be an `if ... else`?


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

https://reviews.llvm.org/D114427

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

[PATCH] D112881: [clang-tidy] Allow disabling integer to floating-point narrowing conversions for cppcoreguidelines-narrowing-conversions

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-narrowingintegertofloatingpoint-option.cpp:12
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:14: warning: narrowing conversion 
from 'unsigned long long' to 'double' [cppcoreguidelines-narrowing-conversions]
+  // DISABLED: No warning for integer to floating-point narrowing conversions 
when WarnOnIntegerToFloatingPointNarrowingConversion = false.
+}

Build is failing because you don't have a CHECK-MESSAGES-DISABLED line anywhere 
in the file.
You could change `// DISABLED:` to `// CHECK-MESSAGES-DISABLED-NOT:` and check 
for the absence of the check warning.


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

https://reviews.llvm.org/D112881

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

[PATCH] D112881: [clang-tidy] Allow disabling integer to floating-point narrowing conversions for cppcoreguidelines-narrowing-conversions

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-narrowingintegertofloatingpoint-option.cpp:12
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:14: warning: narrowing conversion 
from 'unsigned long long' to 'double' [cppcoreguidelines-narrowing-conversions]
+  // DISABLED: No warning for integer to floating-point narrowing conversions 
when WarnOnIntegerToFloatingPointNarrowingConversion = false.
+}

salman-javed-nz wrote:
> Build is failing because you don't have a CHECK-MESSAGES-DISABLED line 
> anywhere in the file.
> You could change `// DISABLED:` to `// CHECK-MESSAGES-DISABLED-NOT:` and 
> check for the absence of the check warning.



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

https://reviews.llvm.org/D112881

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz accepted this revision.
salman-javed-nz added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:335
+  const char *str = "foo";
+  auto s = S(str);
+}

carlosgalvezp wrote:
> salman-javed-nz wrote:
> > Is a test to check `new int(x)` worth including? I see that the cpplint 
> > guys explicitly filter it out of their results.
> Sure, even though I think technically it's not a cast. At least it's not 
> shown as such in the AST.
It's not a cast in the AST, but it's nice to document in the unit test that we 
have considered it and that that intend to treat it no differently to how 
cpplint treats it.


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:342
+  auto w = new int(x);
+}

carlosgalvezp wrote:
> carlosgalvezp wrote:
> > Quuxplusone wrote:
> > > What about
> > > ```
> > > template T foo(int i) { return T(i); }
> > > int main() {
> > > foo>(); // valid, OK(!)
> > > foo(); // valid, not OK
> > > }
> > > ```
> > > What about
> > > ```
> > > struct Widget { Widget(int); };
> > > using T = Widget;
> > > using U = Widget&;
> > > int i = 42;
> > > Widget t = T(i);  // valid, OK?
> > > Widget u = U(i);  // valid C++, should definitely not be OK
> > > ```
> > > https://quuxplusone.github.io/blog/2020/01/22/expression-list-in-functional-cast/
> > Good point, thanks! I've added the first one to the unit test.
> > 
> > Regarding the second check, I'm not sure if it's the scope of this check. 
> > This check does not care whether the constructor of the class is implicit 
> > or not - if you use a class type, then you are calling the constructor so 
> > it's fine. Same goes when it's a reference - in my opinion this check is 
> > not concerned with that.
> > 
> > I definitely see the problems that can arise from the example that you 
> > posted, but maybe it fits better as a separate check in the `bugprone` 
> > category? This check (`google-readability-casting`) is focused only about 
> > avoiding C-style casting, i.e. it's a readability/style/modernize matter 
> > IMO. If cpplint is not diagnosing that, I don't think this check should 
> > either.
> It seems I should be able to just add the second example as a test and 
> clang-tidy would warn but, what should be the fixit for it? A 
> `static_cast` would lead to compiler error (which I personally would 
> gladly take, but I don't know in general if we want clang-tidy to "fix" code 
> leading to compiler errors"). Adding an ad-hoc message for this particular 
> error seems out of the scope of a "readability" check. 
> 
> What do you guys think?
> It seems I should be able to just add the second example as a test and 
> clang-tidy would warn but, what should be the fixit for it?

If I run the second example, but with old style C casts instead:

Input:
```lang=cpp
struct Widget { Widget(int); };
using T = Widget;
using U = Widget&;
int i = 42;
Widget t = (T)(i);
Widget u = (U)(i);
```

Output after fixits:
```lang=cpp
struct Widget { Widget(int); };
using T = Widget;
using U = Widget&;
int i = 42;
Widget t = T(i);
Widget u = (U)(i);
```

I guess the fix `Widget t = T(i);` is OK as it is covered by this exception:
>You may use cast formats like `T(x)` only when `T` is a class type.

For the `Widget u = (U)(i);` line, clang-tidy has warned about it but not 
offered a fix.


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

I think the primary goal is satisfied - in all cases the cast is identified and 
a warning is generated.

For the `Widget&` case, a warning is generated but no fixit is offered, but 
that isn't any worse than the existing C-style cast fixits.
It does sound like a case where offering no fix is better than offering a fix 
that makes things worse.

What would be the right fixit for that anyway?
`Widget u = U(i);   -->   Widget u = T(i);` ?


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.



Comment at: clang-tools-extra/clang-tidy/google/AvoidCStyleCastsCheck.cpp:63-73
+static clang::CharSourceRange getReplaceRange(const ExplicitCastExpr *Expr) {
+  if (const auto *CastExpr = dyn_cast(Expr)) {
+return CharSourceRange::getCharRange(
+CastExpr->getLParenLoc(),
+CastExpr->getSubExprAsWritten()->getBeginLoc());
+  } else if (const auto *CastExpr = dyn_cast(Expr)) {
+return CharSourceRange::getCharRange(CastExpr->getBeginLoc(),

Looks much better than my suggested change. Good stuff.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:139
 
+- Updated ``google-readability-casting`` to diagnose and fix functional casts, 
to achieve feature
+  parity with the corresponding ``cpplint.py`` check.

Should this be like the
```
:doc:`bugprone-suspicious-memory-comparison
  `
```
a few lines above.


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

https://reviews.llvm.org/D114427

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

[PATCH] D114427: [clang-tidy] Warn on functional C-style casts

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

Nothing else that comes to mind that I want to see.
@Quuxplusone are you OK with the latest round of diffs?




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/google-readability-casting.cpp:318
   // FIXME: This should be a static_cast.
   // C HECK-FIXES: auto s5 = static_cast(cr);
   auto s6 = (S)cr;

Isn't this intentional? Isn't it saying what the CHECK-MESSAGES should look 
like in the future once the FIXME is done?


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

https://reviews.llvm.org/D114427

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

[PATCH] D114149: [clang-tidy] Fix pr48613: "llvm-header-guard uses a reserved identifier"

[Salman Javed via Phabricator via cfe-commits]

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

Update "iff" comment based on review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114149

Files:
  clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp
  clang-tools-extra/clang-tidy/utils/HeaderGuard.h
  clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp


Index: clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
@@ -219,6 +219,16 @@
 "", "/llvm-project/clang-tools-extra/clangd/foo.h",
 StringRef("header is missing header guard")));
 
+  // Substitution of characters should not result in a header guard starting
+  // with "_".
+  EXPECT_EQ("#ifndef BAR_H\n"
+"#define BAR_H\n"
+"\n"
+"\n"
+"#endif\n",
+runHeaderGuardCheck("", "include/--bar.h",
+StringRef("header is missing header guard")));
+
 #ifdef WIN32
   // Check interaction with Windows-style path separators (\).
   EXPECT_EQ(
Index: clang-tools-extra/clang-tidy/utils/HeaderGuard.h
===
--- clang-tools-extra/clang-tidy/utils/HeaderGuard.h
+++ clang-tools-extra/clang-tidy/utils/HeaderGuard.h
@@ -38,6 +38,9 @@
   void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override;
 
+  /// Ensure that the provided header guard is a non-reserved identifier.
+  std::string sanitizeHeaderGuard(StringRef Guard);
+
   /// Returns ``true`` if the check should suggest inserting a trailing comment
   /// on the ``#endif`` of the header guard. It will use the same name as
   /// returned by ``HeaderGuardCheck::getHeaderGuard``.
Index: clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp
===
--- clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp
+++ clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp
@@ -164,10 +164,11 @@
  StringRef CurHeaderGuard,
  std::vector &FixIts) {
 std::string CPPVar = Check->getHeaderGuard(FileName, CurHeaderGuard);
+CPPVar = Check->sanitizeHeaderGuard(CPPVar);
 std::string CPPVarUnder = CPPVar + '_';
 
-// Allow a trailing underscore iff we don't have to change the endif 
comment
-// too.
+// Allow a trailing underscore if and only if we don't have to change the
+// endif comment too.
 if (Ifndef.isValid() && CurHeaderGuard != CPPVar &&
 (CurHeaderGuard != CPPVarUnder ||
  wouldFixEndifComment(FileName, EndIf, CurHeaderGuard))) {
@@ -216,6 +217,7 @@
 continue;
 
   std::string CPPVar = Check->getHeaderGuard(FileName);
+  CPPVar = Check->sanitizeHeaderGuard(CPPVar);
   std::string CPPVarUnder = CPPVar + '_'; // Allow a trailing underscore.
   // If there's a macro with a name that follows the header guard 
convention
   // but was not recognized by the preprocessor as a header guard there 
must
@@ -278,6 +280,11 @@
   PP->addPPCallbacks(std::make_unique(PP, this));
 }
 
+std::string HeaderGuardCheck::sanitizeHeaderGuard(StringRef Guard) {
+  // Only reserved identifiers are allowed to start with an '_'.
+  return Guard.drop_while([](char C) { return C == '_'; }).str();
+}
+
 bool HeaderGuardCheck::shouldSuggestEndifComment(StringRef FileName) {
   return utils::isFileExtension(FileName, HeaderFileExtensions);
 }


Index: clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
@@ -219,6 +219,16 @@
 "", "/llvm-project/clang-tools-extra/clangd/foo.h",
 StringRef("header is missing header guard")));
 
+  // Substitution of characters should not result in a header guard starting
+  // with "_".
+  EXPECT_EQ("#ifndef BAR_H\n"
+"#define BAR_H\n"
+"\n"
+"\n"
+"#endif\n",
+runHeaderGuardCheck("", "include/--bar.h",
+StringRef("header is missing header guard")));
+
 #ifdef WIN32
   // Check interaction with Windows-style path separators (\).
   EXPECT_EQ(
Index: clang-tools-extra/clang-tidy/utils/HeaderGuard.h
===
--- clang-tools-extra/clang-tidy/utils/HeaderGuard.h
+++ clang-tools-extra/clang-tidy/utils/HeaderGuard.h
@@ -38,6 +38,9 @@
   void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
 

[PATCH] D114149: [clang-tidy] Fix pr48613: "llvm-header-guard uses a reserved identifier"

[Salman Javed via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc7aa358798e6: [clang-tidy] Fix pr48613: 
"llvm-header-guard uses a reserved identifier" (authored by 
salman-javed-nz).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114149

Files:
  clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp
  clang-tools-extra/clang-tidy/utils/HeaderGuard.h
  clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp


Index: clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
@@ -219,6 +219,16 @@
 "", "/llvm-project/clang-tools-extra/clangd/foo.h",
 StringRef("header is missing header guard")));
 
+  // Substitution of characters should not result in a header guard starting
+  // with "_".
+  EXPECT_EQ("#ifndef BAR_H\n"
+"#define BAR_H\n"
+"\n"
+"\n"
+"#endif\n",
+runHeaderGuardCheck("", "include/--bar.h",
+StringRef("header is missing header guard")));
+
 #ifdef WIN32
   // Check interaction with Windows-style path separators (\).
   EXPECT_EQ(
Index: clang-tools-extra/clang-tidy/utils/HeaderGuard.h
===
--- clang-tools-extra/clang-tidy/utils/HeaderGuard.h
+++ clang-tools-extra/clang-tidy/utils/HeaderGuard.h
@@ -38,6 +38,9 @@
   void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
Preprocessor *ModuleExpanderPP) override;
 
+  /// Ensure that the provided header guard is a non-reserved identifier.
+  std::string sanitizeHeaderGuard(StringRef Guard);
+
   /// Returns ``true`` if the check should suggest inserting a trailing comment
   /// on the ``#endif`` of the header guard. It will use the same name as
   /// returned by ``HeaderGuardCheck::getHeaderGuard``.
Index: clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp
===
--- clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp
+++ clang-tools-extra/clang-tidy/utils/HeaderGuard.cpp
@@ -164,10 +164,11 @@
  StringRef CurHeaderGuard,
  std::vector &FixIts) {
 std::string CPPVar = Check->getHeaderGuard(FileName, CurHeaderGuard);
+CPPVar = Check->sanitizeHeaderGuard(CPPVar);
 std::string CPPVarUnder = CPPVar + '_';
 
-// Allow a trailing underscore iff we don't have to change the endif 
comment
-// too.
+// Allow a trailing underscore if and only if we don't have to change the
+// endif comment too.
 if (Ifndef.isValid() && CurHeaderGuard != CPPVar &&
 (CurHeaderGuard != CPPVarUnder ||
  wouldFixEndifComment(FileName, EndIf, CurHeaderGuard))) {
@@ -216,6 +217,7 @@
 continue;
 
   std::string CPPVar = Check->getHeaderGuard(FileName);
+  CPPVar = Check->sanitizeHeaderGuard(CPPVar);
   std::string CPPVarUnder = CPPVar + '_'; // Allow a trailing underscore.
   // If there's a macro with a name that follows the header guard 
convention
   // but was not recognized by the preprocessor as a header guard there 
must
@@ -278,6 +280,11 @@
   PP->addPPCallbacks(std::make_unique(PP, this));
 }
 
+std::string HeaderGuardCheck::sanitizeHeaderGuard(StringRef Guard) {
+  // Only reserved identifiers are allowed to start with an '_'.
+  return Guard.drop_while([](char C) { return C == '_'; }).str();
+}
+
 bool HeaderGuardCheck::shouldSuggestEndifComment(StringRef FileName) {
   return utils::isFileExtension(FileName, HeaderFileExtensions);
 }


Index: clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
@@ -219,6 +219,16 @@
 "", "/llvm-project/clang-tools-extra/clangd/foo.h",
 StringRef("header is missing header guard")));
 
+  // Substitution of characters should not result in a header guard starting
+  // with "_".
+  EXPECT_EQ("#ifndef BAR_H\n"
+"#define BAR_H\n"
+"\n"
+"\n"
+"#endif\n",
+runHeaderGuardCheck("", "include/--bar.h",
+StringRef("header is missing header guard")));
+
 #ifdef WIN32
   // Check interaction with Windows-style path separators (\).
   EXPECT_EQ(
Index: clang-tools-extra/clang-tidy/utils/HeaderGuard.h
===
--- clang-tools-extra/clang-tidy/utils/HeaderGuard.h
+++ clang-tools-extra/clang-tidy/utils/HeaderGuard.h
@@ -38,6 +38,9 @@
   void re

[PATCH] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

Will aim to review and come back to you in a couple of days.


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

https://reviews.llvm.org/D113422

___
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

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

Thanks for the investigation. Just exploring the options...

With regards to reverting it, is the cat out of the bag? I already see some 
usage of the NOLINTBEGIN feature in other projects.
There's also a NOLINT check globbing feature that builds on top of this, so 
it's not as simple as just backing out one commit.

I can have a go at coming up with a solution where it does some caching of the 
NOLINT marker positions when a diagnostic is generated in a file. That would 
probably help a lot with the performance. It wouldn't be too complicated to 
implement either. Would you be happy with something like that?

If the current code stayed in the main repo, how long could you afford to wait 
for me to provide a 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

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

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

> I can have a go at coming up with a solution where it does caching of the 
> NOLINT marker positions when a diagnostic is generated in a file. That would 
> probably help a lot with the performance. It wouldn't be too complicated to 
> implement either. Would you be happy with something like that?

That was me just thinking out loud. If you have other approaches, I would be 
keen to hear them.


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

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

In D108560#3167883 , @aaron.ballman 
wrote:

> In D108560#3167847 , 
> @salman-javed-nz wrote:
>
>> If the current code stayed in the main repo, how long could you afford to 
>> wait for me to provide a patch?
>
> I would say that for me personally, at a maximum, we have to either fix or 
> revert before this goes out in a release of Clang (because Clang 13 did not 
> have support for NOLINTBEGIN/NOLINTEND, so this is technically unreleased 
> functionality).

I see. Under normal circumstances, which release would this patch be going 
into? Clang 14, not the next 13 point release, right?


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

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

In D108560#3168006 , @ymandel wrote:

> In D108560#3167847 , 
> @salman-javed-nz wrote:
>
>> With regards to reverting it, is the cat out of the bag? I already see some 
>> usage of the NOLINTBEGIN feature in other projects.
>> There's also a NOLINT check globbing feature that builds on top of this, so 
>> it's not as simple as just backing out one commit.

If this is the approach we take, I prefer the logic to be switched around: the 
NOLINT block feature should be enabled by default. Once (the worst of) this 
performance issue is resolved, it would be nice from a usability perspective if 
the user did not have to explicitly opt in to use the feature.

> I wonder if we change my patch so that it leaves the feature enabled by 
> default, it would still allow us to modify internal tools to disable the 
> feature in the interim.

Yes, that sounds better to me.

> Also, I'm concerned that even an optimized implementation of this might have 
> noticable (if much smaller) impact, since it will be adding an O(n) pass to 
> clang-tidy. Compared with the cost of building the AST this may be trivial, 
> but still. So, the parameter I added to the constructor might be valuable 
> long term to allow clients of the class to disable this feature, even if we 
> don't want to expose it as a CLI flag (for the reasons Aaron mentioned above).

I'll do my best to make this feature as efficient as possible. There will 
always be some performance impact - after all, we are asking clang-tidy to do 
more than it did before. I wonder how much the execution time increases when a 
new clang-tidy check is added.
I don't have the answer to what's the right balance between performance and 
functionality. The NOLINT block feature has been something people have been 
asking for for a long time. How much performance hit are they willing to take 
to make suppressing clang-tidy warnings a bit easier?


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

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

Update: OK I see you have already changed your patch to make the feature 
enabled by default.
As far as the performance is concerned, I will have a go at improving it and 
hope to have something to share next week. How does that sound?


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] D113422: [clang-tidy][NFC] Move CachedGlobList to GlobList.h

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz accepted this revision.
salman-javed-nz added a comment.
This revision is now accepted and ready to land.

LGTM. The use of `mutable` with public inheritence is all over the 
clang-tools-extra code. See `class SwapIndex : public SymbolIndex`.


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

https://reviews.llvm.org/D113422

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

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

[Salman Javed via Phabricator via cfe-commits]

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

Added release note.

(Also taking the opportunity to run the patch through the pre-merge build bot 
one last time.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126138

Files:
  clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp


Index: clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
@@ -1,5 +1,5 @@
 // REQUIRES: static-analyzer
-// RUN: %check_clang_tidy %s 
google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays
 %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
+// RUN: %check_clang_tidy %s 
google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays,cppcoreguidelines-pro-type-member-init
 %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
 
 #include "trigger_warning.h"
 void I(int& Out) {
@@ -96,6 +96,23 @@
 #define MACRO_NOLINT class G { G(int i); }; // NOLINT
 MACRO_NOLINT
 
+// Check that we can suppress diagnostics about macro arguments (as opposed to
+// diagnostics about the macro contents itself).
+#define MACRO_SUPPRESS_DIAG_FOR_ARG_1(X)\
+  class X { \
+X(int i); /* NOLINT(google-explicit-constructor) */ \
+  };
+
+MACRO_SUPPRESS_DIAG_FOR_ARG_1(G1)
+
+#define MACRO_SUPPRESS_DIAG_FOR_ARG_2(X)  \
+  struct X { /* NOLINT(cppcoreguidelines-pro-type-member-init) */ \
+int a = 0;\
+int b;\
+  };
+
+MACRO_SUPPRESS_DIAG_FOR_ARG_2(G2)
+
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
@@ -116,4 +133,4 @@
 int array3[10];  // 
NOLINT(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
 int array4[10];  // NOLINT(*-avoid-c-arrays)
 
-// CHECK-MESSAGES: Suppressed 34 warnings (34 NOLINT)
+// CHECK-MESSAGES: Suppressed 36 warnings (36 NOLINT)
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -106,6 +106,10 @@
   means it is advised to use YAML's block style initiated by the pipe 
character `|` for the `Checks`
   section in order to benefit from the easier syntax that works without commas.
 
+- Fixed a regression introduced in clang-tidy 14.0.0, which prevented NOLINTs
+  from suppressing diagnostics associated with macro arguments. This fixes
+  `Issue 55134 `_.
+
 New checks
 ^^
 
Index: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
===
--- clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
+++ clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
@@ -266,7 +266,7 @@
   return true;
 if (!DiagLoc.isMacroID())
   return false;
-DiagLoc = SrcMgr.getImmediateMacroCallerLoc(DiagLoc);
+DiagLoc = SrcMgr.getImmediateExpansionRange(DiagLoc).getBegin();
   }
   return false;
 }


Index: clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
@@ -1,5 +1,5 @@
 // REQUIRES: static-analyzer
-// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
+// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays,cppcoreguidelines-pro-type-member-init %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
 
 #include "trigger_warning.h"
 void I(int& Out) {
@@ -96,6 +96,23 @@
 #define MACRO_NOLINT class G { G(int i); }; // NOLINT
 MACRO_NOLINT
 
+// Check that we can suppress diagnostics about macro arguments (as opposed to
+// diagnostics about the macro contents itself).
+#define MACRO_SUPPRESS_DIAG_FOR_ARG_1(X)\
+  class X { \
+X(int i); /* NOLINT(google-explicit-constructor) */ \
+  };
+

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

[Salman Javed via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9ff4f2dfea63: [clang-tidy] Fix #55134 (regression introduced 
by 5da7c04) (authored by salman-javed-nz).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126138

Files:
  clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp


Index: clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
@@ -1,5 +1,5 @@
 // REQUIRES: static-analyzer
-// RUN: %check_clang_tidy %s 
google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays
 %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
+// RUN: %check_clang_tidy %s 
google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays,cppcoreguidelines-pro-type-member-init
 %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
 
 #include "trigger_warning.h"
 void I(int& Out) {
@@ -96,6 +96,23 @@
 #define MACRO_NOLINT class G { G(int i); }; // NOLINT
 MACRO_NOLINT
 
+// Check that we can suppress diagnostics about macro arguments (as opposed to
+// diagnostics about the macro contents itself).
+#define MACRO_SUPPRESS_DIAG_FOR_ARG_1(X)\
+  class X { \
+X(int i); /* NOLINT(google-explicit-constructor) */ \
+  };
+
+MACRO_SUPPRESS_DIAG_FOR_ARG_1(G1)
+
+#define MACRO_SUPPRESS_DIAG_FOR_ARG_2(X)  \
+  struct X { /* NOLINT(cppcoreguidelines-pro-type-member-init) */ \
+int a = 0;\
+int b;\
+  };
+
+MACRO_SUPPRESS_DIAG_FOR_ARG_2(G2)
+
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
@@ -116,4 +133,4 @@
 int array3[10];  // 
NOLINT(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
 int array4[10];  // NOLINT(*-avoid-c-arrays)
 
-// CHECK-MESSAGES: Suppressed 34 warnings (34 NOLINT)
+// CHECK-MESSAGES: Suppressed 36 warnings (36 NOLINT)
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -106,6 +106,10 @@
   means it is advised to use YAML's block style initiated by the pipe 
character `|` for the `Checks`
   section in order to benefit from the easier syntax that works without commas.
 
+- Fixed a regression introduced in clang-tidy 14.0.0, which prevented NOLINTs
+  from suppressing diagnostics associated with macro arguments. This fixes
+  `Issue 55134 `_.
+
 New checks
 ^^
 
Index: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
===
--- clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
+++ clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
@@ -266,7 +266,7 @@
   return true;
 if (!DiagLoc.isMacroID())
   return false;
-DiagLoc = SrcMgr.getImmediateMacroCallerLoc(DiagLoc);
+DiagLoc = SrcMgr.getImmediateExpansionRange(DiagLoc).getBegin();
   }
   return false;
 }


Index: clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
@@ -1,5 +1,5 @@
 // REQUIRES: static-analyzer
-// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
+// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays,cppcoreguidelines-pro-type-member-init %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
 
 #include "trigger_warning.h"
 void I(int& Out) {
@@ -96,6 +96,23 @@
 #define MACRO_NOLINT class G { G(int i); }; // NOLINT
 MACRO_NOLINT
 
+// Check that we can suppress diagnostics about macro arguments (as opposed to
+// diagnostics about the macro contents itself).
+#define MACRO_SUPPRESS_DIAG_FOR_ARG_1(X)\
+  class X { \
+X(int i); /* 

[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

In D116085#3257210 , @carlosgalvezp 
wrote:

> Amazing job @salman-javed-nz ! Here's some initial comments. Reviewing the 
> `NoLintPragmaHandler.cpp` will take some more time from me. It would have 
> been easier to see the diff if the contents had been moved as an NFC patch to 
> a separate file, and then applying the optimizations in this patch. But it's 
> done now so I'll deal with it :)

Thank you :)

You might find the previous revision of this patch useful then 
(https://reviews.llvm.org/D116085?id=395606). In this earlier revision I made 
the change in the original file `ClangTidyDiagnosticHandler.cpp`.
There's been some refinements to that code in the subsequent revisions (the 
subsequent revisions aren't merely just moving the changes to a separate file) 
but it's still useful as a way to get the general gist of the new NOLINT 
tokenization and location caching process.




Comment at: clang-tools-extra/clang-tidy/CMakeLists.txt:20
   GlobList.cpp
+  NoLintPragmaHandler.cpp
 

carlosgalvezp wrote:
> I think "Pragma" is a very specific term, used for example in `#pragma gcc 
> diagnostic` or `// IWYU pragma: keep`, but in `clang-tidy` we don't use the 
> word `pragma`, so that might be confusing. What about renaming it to 
> `NoLintHandler.cpp` or `NoLintDirectiveHandler.cpp`?
Your suggestions sound better. I'm not attached to the name 
`NoLintPragmaHandler`.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:125
+const Diagnostic &Info,
+SmallVectorImpl &NoLintErrors);
+

carlosgalvezp wrote:
> Why not `SmallVector`? Sounds like the `Impl` is some "private" 
> implementation?
`SmallVectorImpl` is the recommended type to use for output params.
- `SmallVector` is actually `SmallVectorImpl` 
under the hood.
- By declaring the function param as `SmallVectorImpl` I'm saying that I don't 
care about the size of the vector that the caller passes in.

See https://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h
```
Even though it has “Impl” in the name, SmallVectorImpl is widely used and is no 
longer “private to the implementation”.
```



Comment at: clang-tools-extra/clang-tidy/NoLintPragmaHandler.h:18
+namespace llvm {
+template  class SmallVectorImpl;
+} // namespace llvm

carlosgalvezp wrote:
> Why not `SmallVector`?
See my other comment about `SmallVectorImpl`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116085

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

[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz updated this revision to Diff 401576.
salman-javed-nz edited the summary of this revision.
salman-javed-nz added a comment.

Update according to review (rename NoLintPragmaHandler class to 
NoLintDirectiveHandler)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116085

Files:
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
  clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h
  clang-tools-extra/clangd/ParsedAST.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/1st-translation-unit.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/2nd-translation-unit.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-LIFO.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-all-end-glob.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-all-end-specific.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-glob-end-all.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-glob-end-specific.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-global-end-specific.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-multiple-end-single.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-single-end-multiple.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-all.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-glob.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-global.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-multiple-TUs.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-typo-in-check-name.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
@@ -58,30 +58,24 @@
 // NOLINTEND(some-other-check)
 // NOLINTEND(google-explicit-constructor)
 
-// NOLINTBEGIN(google-explicit-constructor)
-// NOLINTBEGIN(some-other-check)
-class C7 { C7(int i); };
-// NOLINTEND(google-explicit-constructor)
-// NOLINTEND(some-other-check)
-
 // NOLINTBEGIN(google-explicit-constructor)
 // NOLINTBEGIN
-class C8 { C8(int i); };
+class C7 { C7(int i); };
 // NOLINTEND
 // NOLINTEND(google-explicit-constructor)
 
 // NOLINTBEGIN
 // NOLINTBEGIN(google-explicit-constructor)
-class C9 { C9(int i); };
+class C8 { C8(int i); };
 // NOLINTEND(google-explicit-constructor)
 // NOLINTEND
 
 // NOLINTBEGIN(not-closed-bracket-is-treated-as-skip-all
-class C10 { C10(int i); };
+class C9 { C9(int i); };
 // NOLINTEND(not-closed-bracket-is-treated-as-skip-all
 
 // NOLINTBEGIN without-brackets-skip-all, another-check
-class C11 { C11(int i); };
+class C10 { C10(int i); };
 // NOLINTEND without-brackets-skip-all, another-check
 
 #define MACRO(X) class X { X(int i); };
@@ -114,28 +108,28 @@
 MACRO_NO_LINT_INSIDE_MACRO
 
 // NOLINTBEGIN(google*)
-class C12 { C12(int i); };
+class C11 { C11(int i); };
 // NOLINTEND(google*)
 
 // NOLINTBEGIN(*explicit-constructor)
-class C15 { C15(int i); };
+class C12 { C12(int i); };
 // NOLINTEND(*explicit-constructor)
 
 // NOLINTBEGIN(*explicit*)
-class C16 { C16(int i); };
+class C13 { C13(int i); };
 // NOLINTEND(*explicit*)
 
 // NOLINTBEGIN(-explicit-constructor)
-class C17 { C17(int x); };
+class C14 { C14(int x); };
 // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: single-argument constructors must be marked explicit
 // NOLINTEND(-explicit-constructor)
 
 // NOLINTBEGIN(google*,-google*)
-class C18 { C18(int x); };
+class C15 { C15(int x); };
 // NOLINTEND(google*,-google*)
 
 // NOLINTBEGIN(*,-google*)
-class C19 { C19(int x); };
+class C16 { C16(int x); };
 // NOLINTEND(*,-google*)
 
 int array1[10];
@@ -154,4 +148,4 @@
 int array4[10];
 // NOLINTEND(*-avoid-c-arrays)
 
-// CHECK-MESSAGES: Suppressed 27 warnings (27 NOLINT).
+// CHECK-MESSAGES: Suppressed 26 warnings (26 NOLINT).
Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-typo-in-check-name.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolintb

[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

Thank you very much to both of you for having a look at the patch. Yes, I agree 
it is a large patch, and I could have done a better job splitting it up into 
more manageable chunks.

One reason this change is so big is because I set myself an ambitious target 
for the performance gains I wanted to achieve in this patch. After spending 
more time with the current implementation in `master`/`main`, I began to 
realize how expensive it really is. Others have too, as demonstrated by the 
fact that multiple commits have been made to disable the functionality in 
downstream projects. So wherever I saw a more efficient way of doing things, I 
didn't want to rule out making a change, even if it means parts of the code had 
to be refactored or rewritten.

There is a balance I have to strike between achieving my primary objective and 
minimizing the impact to the code base, and I admit I haven't got it right so 
far. We can discuss where exactly the balance should be, but let me tidy up the 
more egregious instances of unnecessary code first, like the public accessors 
that don't hold their weight that you mentioned.

To make your lives easier, I will address your requested changes as separate 
updates to this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116085

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

[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz updated this revision to Diff 402308.
salman-javed-nz changed the visibility from "Only User: salman-javed-nz (Salman 
Javed)" to "Public (No Login Required)".
salman-javed-nz added a comment.

Pass the `NoLintErrors` SmallVector all the way through the call stack, instead 
of storing it in the class


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116085

Files:
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
  clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h
  clang-tools-extra/clangd/ParsedAST.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/1st-translation-unit.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/2nd-translation-unit.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-LIFO.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-all-end-glob.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-all-end-specific.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-glob-end-all.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-glob-end-specific.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-global-end-specific.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-multiple-end-single.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-single-end-multiple.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-all.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-glob.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-global.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-multiple-TUs.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-typo-in-check-name.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
@@ -58,30 +58,24 @@
 // NOLINTEND(some-other-check)
 // NOLINTEND(google-explicit-constructor)
 
-// NOLINTBEGIN(google-explicit-constructor)
-// NOLINTBEGIN(some-other-check)
-class C7 { C7(int i); };
-// NOLINTEND(google-explicit-constructor)
-// NOLINTEND(some-other-check)
-
 // NOLINTBEGIN(google-explicit-constructor)
 // NOLINTBEGIN
-class C8 { C8(int i); };
+class C7 { C7(int i); };
 // NOLINTEND
 // NOLINTEND(google-explicit-constructor)
 
 // NOLINTBEGIN
 // NOLINTBEGIN(google-explicit-constructor)
-class C9 { C9(int i); };
+class C8 { C8(int i); };
 // NOLINTEND(google-explicit-constructor)
 // NOLINTEND
 
 // NOLINTBEGIN(not-closed-bracket-is-treated-as-skip-all
-class C10 { C10(int i); };
+class C9 { C9(int i); };
 // NOLINTEND(not-closed-bracket-is-treated-as-skip-all
 
 // NOLINTBEGIN without-brackets-skip-all, another-check
-class C11 { C11(int i); };
+class C10 { C10(int i); };
 // NOLINTEND without-brackets-skip-all, another-check
 
 #define MACRO(X) class X { X(int i); };
@@ -114,28 +108,28 @@
 MACRO_NO_LINT_INSIDE_MACRO
 
 // NOLINTBEGIN(google*)
-class C12 { C12(int i); };
+class C11 { C11(int i); };
 // NOLINTEND(google*)
 
 // NOLINTBEGIN(*explicit-constructor)
-class C15 { C15(int i); };
+class C12 { C12(int i); };
 // NOLINTEND(*explicit-constructor)
 
 // NOLINTBEGIN(*explicit*)
-class C16 { C16(int i); };
+class C13 { C13(int i); };
 // NOLINTEND(*explicit*)
 
 // NOLINTBEGIN(-explicit-constructor)
-class C17 { C17(int x); };
+class C14 { C14(int x); };
 // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: single-argument constructors must be marked explicit
 // NOLINTEND(-explicit-constructor)
 
 // NOLINTBEGIN(google*,-google*)
-class C18 { C18(int x); };
+class C15 { C15(int x); };
 // NOLINTEND(google*,-google*)
 
 // NOLINTBEGIN(*,-google*)
-class C19 { C19(int x); };
+class C16 { C16(int x); };
 // NOLINTEND(*,-google*)
 
 int array1[10];
@@ -154,4 +148,4 @@
 int array4[10];
 // NOLINTEND(*-avoid-c-arrays)
 
-// CHECK-MESSAGES: Suppressed 27 warnings (27 NOLINT).
+// CHECK-MESSAGES: Suppressed 26 warnings (26 NOLINT).
Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-typo-in-check-name.cpp

[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

[Salman Javed via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:84
+   bool AllowEnablingAnalyzerAlphaCheckers = false,
+   bool AllowIO = true, bool EnableNoLintBlocks = true);
+

kadircet wrote:
> why are we moving these two parameters here?
Parameters moved back to `shouldSuppressDiagnostic()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116085

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

[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

[Salman Javed via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/NoLintPragmaHandler.cpp:63
+// as parsed from the file's character contents.
+class NoLintToken {
+public:

kadircet wrote:
> kadircet wrote:
> > why not just a basic struct ? i don't think all of these 
> > accessors/constructors carry their weight (as mentioned below once the 
> > `ChecksGlob` is not lazily computed, there should be no need for any of 
> > these).
> can we please inline the definitions (if there's a good reason to stick with 
> the class)?
I've removed as many accessors and constructors as I can from this class. I 
believe there's still some value in hiding the `Optional Checks` 
and `CachedGlobList ChecksGlob` behind methods, because if you modify one, you 
must modify the other for the `NoLintToken` object to make sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116085

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

[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz marked an inline comment as done.
salman-javed-nz added inline comments.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:448
+/// this line.
+static std::pair getLineStartAndEnd(StringRef S, size_t From) {
+  size_t StartPos = S.find_last_of('\n', From) + 1;

carlosgalvezp wrote:
> Don't we usually use `SourceLocation` objects for this?
> Don't we usually use `SourceLocation` objects for this?
Considering that we're working with a string buffer of the file contents, I 
think using buffer indices of type size_t is appropriate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116085

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

[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:349
+  if (Context.shouldSuppressDiagnostic(DiagLevel, Info, SuppressionErrors,
+   EnableNolintBlocks)) {
 ++Context.Stats.ErrorsIgnoredNOLINT;

This is NOT a tab character, even though it looks like one on Phabricator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116085

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

[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

Every review comment so far should be addressed now, with the exception of the 
following 2 points.




Comment at: clang-tools-extra/clang-tidy/NoLintPragmaHandler.cpp:420
+  // file if it is a .
+  Optional FileName = SrcMgr.getNonBuiltinFilenameForID(File);
+  if (!FileName)

kadircet wrote:
> filenames in source manager could be misleading depending on how the file is 
> accessed (there might be an override, symlinks, includemaps etc.) and 
> different fileids can also refer to same file (e.g. when header is not 
> include guarded). so both can result in reading the same file contents 
> multiple times, but at least fileids are not strings so should be better keys 
> for the cache && get rid of this step around fetching the filename from 
> fileid.
> 
> Hence can we switch the cache from stringmap to a `densemap vector>` instead.
> filenames in source manager could be misleading depending on how the file is 
> accessed (there might be an override, symlinks, includemaps etc.) and 
> different fileids can also refer to same file (e.g. when header is not 
> include guarded). so both can result in reading the same file contents 
> multiple times, but at least fileids are not strings so should be better keys 
> for the cache && get rid of this step around fetching the filename from 
> fileid.
> 
> Hence can we switch the cache from stringmap to a `densemap vector>` instead.

I used FileIDs in an earlier attempt at this patch, but I had issues when 
specifying multiple input files to clang-tidy on the command line, e.g. 
`clang-tidy file1.cpp file2.cpp`. The analysis of each file begins with a fresh 
instance of SourceManager, so both file1.cpp and file2.cpp have a FileID of 1. 
It looks to me that I would need to clear the NoLintBlock cache each time a new 
SourceManager is used.

This is what the `nolintbeginend-multiple-TUs.cpp` test is all about.

The file path is a SourceManager-independent means of identifying the file for 
use in a map, so that's why it's being used. What are your thoughts on what map 
key to use? Neither looks ideal to me.




Comment at: clang-tools-extra/clang-tidy/NoLintPragmaHandler.cpp:469
+  (NoLint.type() == NoLintType::NoLintBegin)
+  ? ("unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINT"
+ "END' comment")

kadircet wrote:
> maybe just `'NOLINTBEGIN' comment without a matching 'NOLINTEND' comment`, 
> vice versa for the other case.
> maybe just `'NOLINTBEGIN' comment without a matching 'NOLINTEND' comment`, 
> vice versa for the other case.
Could we do this in another patch? Quite a number of unit tests will need 
updating.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116085

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

[PATCH] D117857: [clang-tidy] Remove gsl::at suggestion from cppcoreguidelines-pro-bounds-constant-array-index

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz accepted this revision.
salman-javed-nz added a comment.
This revision is now accepted and ready to land.

LGTM besides a couple of nits. I think the diagnostic message should just say 
what the problem is, not what the fix is - that's what the FixitHint is for.




Comment at: clang-tools-extra/docs/ReleaseNotes.rst:163
+- Removed suggestion ``use gsl::at`` from warning message in the
+  ``cppcoreguidelines-pro-bounds-constant-array-index`` check, since that's not
+  a requirement from the CppCoreGuidelines. This allows people to choose





Comment at: clang-tools-extra/docs/ReleaseNotes.rst:164
+  ``cppcoreguidelines-pro-bounds-constant-array-index`` check, since that's not
+  a requirement from the CppCoreGuidelines. This allows people to choose
+  their own safe indexing strategy. The fix-it is kept for those who want to

To match "C++ Core Guidelines" in the previous bullet point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117857

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

[PATCH] D117939: [clang-tidy] Add more documentation about check development

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.



Comment at: clang-tools-extra/docs/clang-tidy/Contributing.rst:76
+When you `configure the CMake build 
`_,
+make sure that you enable the ``clang-tools-extra`` project to build 
:program:`clang-tidy`.
+Since your new check will have associated documentation, you will also want to 
install

Don't you need to enable both clang and clang-tools-extra? Otherwise the 
clang-tidy CMake target doesn't appear. That has been my experience.


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

https://reviews.llvm.org/D117939

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

[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

[Salman Javed via Phabricator via cfe-commits]

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

`ClangTidyContext::shouldSuppressDiagnostic()`:

- Hook up `AllowIO` and `EnableNoLintBlocks` to 
`NoLintDirectiveHandler::shouldSuppress()`

`NoLintToken`:
-Remove copy ctor and assignment operator. Class is now move-only.

`getNoLints()`:

- Use `llvm::StringLiteral` for string literal
- Break out of the loop once there are no more NOLINT strings
- Determine NOLINT checks string inline instead of in a lamda
- Remove the `NoLintToken()` in the call to `emplace_back()`

`NoLintDirectiveHandler`:

- `std::make_unique` --> `std::make_unique`
- Remove default values for `AllowIO` and `EnableNoLintBlocks` parameters


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116085

Files:
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
  clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h
  clang-tools-extra/clangd/ParsedAST.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/1st-translation-unit.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/2nd-translation-unit.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-LIFO.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-all-end-glob.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-all-end-specific.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-glob-end-all.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-glob-end-specific.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-global-end-specific.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-multiple-end-single.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-single-end-multiple.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-all.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-glob.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-global.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-multiple-TUs.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-typo-in-check-name.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
@@ -58,30 +58,24 @@
 // NOLINTEND(some-other-check)
 // NOLINTEND(google-explicit-constructor)
 
-// NOLINTBEGIN(google-explicit-constructor)
-// NOLINTBEGIN(some-other-check)
-class C7 { C7(int i); };
-// NOLINTEND(google-explicit-constructor)
-// NOLINTEND(some-other-check)
-
 // NOLINTBEGIN(google-explicit-constructor)
 // NOLINTBEGIN
-class C8 { C8(int i); };
+class C7 { C7(int i); };
 // NOLINTEND
 // NOLINTEND(google-explicit-constructor)
 
 // NOLINTBEGIN
 // NOLINTBEGIN(google-explicit-constructor)
-class C9 { C9(int i); };
+class C8 { C8(int i); };
 // NOLINTEND(google-explicit-constructor)
 // NOLINTEND
 
 // NOLINTBEGIN(not-closed-bracket-is-treated-as-skip-all
-class C10 { C10(int i); };
+class C9 { C9(int i); };
 // NOLINTEND(not-closed-bracket-is-treated-as-skip-all
 
 // NOLINTBEGIN without-brackets-skip-all, another-check
-class C11 { C11(int i); };
+class C10 { C10(int i); };
 // NOLINTEND without-brackets-skip-all, another-check
 
 #define MACRO(X) class X { X(int i); };
@@ -114,28 +108,28 @@
 MACRO_NO_LINT_INSIDE_MACRO
 
 // NOLINTBEGIN(google*)
-class C12 { C12(int i); };
+class C11 { C11(int i); };
 // NOLINTEND(google*)
 
 // NOLINTBEGIN(*explicit-constructor)
-class C15 { C15(int i); };
+class C12 { C12(int i); };
 // NOLINTEND(*explicit-constructor)
 
 // NOLINTBEGIN(*explicit*)
-class C16 { C16(int i); };
+class C13 { C13(int i); };
 // NOLINTEND(*explicit*)
 
 // NOLINTBEGIN(-explicit-constructor)
-class C17 { C17(int x); };
+class C14 { C14(int x); };
 // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: single-argument constructors must be marked explicit
 // NOLINTEND(-explicit-constructor)
 
 // NOLINTBEGIN(google*,-google*)
-class C18 { C18(int x); };
+class C15 { C15(int x); };
 // NOLINTEND(google*,-google*)
 
 // NOLINTBEGIN(*,-google*)
-class C19 { C19(int x); };

[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

[Salman Javed via Phabricator via cfe-commits]

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

Review comment: 
`formNoLintBlocks()` - drop the `&` so the vector must be std::moved in


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116085

Files:
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
  clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h
  clang-tools-extra/clangd/ParsedAST.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/1st-translation-unit.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/2nd-translation-unit.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-LIFO.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-all-end-glob.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-all-end-specific.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-glob-end-all.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-glob-end-specific.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-global-end-specific.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-multiple-end-single.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-single-end-multiple.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-all.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-glob.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-global.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-multiple-TUs.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-typo-in-check-name.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
@@ -58,30 +58,24 @@
 // NOLINTEND(some-other-check)
 // NOLINTEND(google-explicit-constructor)
 
-// NOLINTBEGIN(google-explicit-constructor)
-// NOLINTBEGIN(some-other-check)
-class C7 { C7(int i); };
-// NOLINTEND(google-explicit-constructor)
-// NOLINTEND(some-other-check)
-
 // NOLINTBEGIN(google-explicit-constructor)
 // NOLINTBEGIN
-class C8 { C8(int i); };
+class C7 { C7(int i); };
 // NOLINTEND
 // NOLINTEND(google-explicit-constructor)
 
 // NOLINTBEGIN
 // NOLINTBEGIN(google-explicit-constructor)
-class C9 { C9(int i); };
+class C8 { C8(int i); };
 // NOLINTEND(google-explicit-constructor)
 // NOLINTEND
 
 // NOLINTBEGIN(not-closed-bracket-is-treated-as-skip-all
-class C10 { C10(int i); };
+class C9 { C9(int i); };
 // NOLINTEND(not-closed-bracket-is-treated-as-skip-all
 
 // NOLINTBEGIN without-brackets-skip-all, another-check
-class C11 { C11(int i); };
+class C10 { C10(int i); };
 // NOLINTEND without-brackets-skip-all, another-check
 
 #define MACRO(X) class X { X(int i); };
@@ -114,28 +108,28 @@
 MACRO_NO_LINT_INSIDE_MACRO
 
 // NOLINTBEGIN(google*)
-class C12 { C12(int i); };
+class C11 { C11(int i); };
 // NOLINTEND(google*)
 
 // NOLINTBEGIN(*explicit-constructor)
-class C15 { C15(int i); };
+class C12 { C12(int i); };
 // NOLINTEND(*explicit-constructor)
 
 // NOLINTBEGIN(*explicit*)
-class C16 { C16(int i); };
+class C13 { C13(int i); };
 // NOLINTEND(*explicit*)
 
 // NOLINTBEGIN(-explicit-constructor)
-class C17 { C17(int x); };
+class C14 { C14(int x); };
 // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: single-argument constructors must be marked explicit
 // NOLINTEND(-explicit-constructor)
 
 // NOLINTBEGIN(google*,-google*)
-class C18 { C18(int x); };
+class C15 { C15(int x); };
 // NOLINTEND(google*,-google*)
 
 // NOLINTBEGIN(*,-google*)
-class C19 { C19(int x); };
+class C16 { C16(int x); };
 // NOLINTEND(*,-google*)
 
 int array1[10];
@@ -154,4 +148,4 @@
 int array4[10];
 // NOLINTEND(*-avoid-c-arrays)
 
-// CHECK-MESSAGES: Suppressed 27 warnings (27 NOLINT).
+// CHECK-MESSAGES: Suppressed 26 warnings (26 NOLINT).
Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-typo-in-check-name.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-typo-in-check-name.cpp
+++ clang-tools-extra/

[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

[Salman Javed via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG19eaad94c47f: [clang-tidy] Cache the locations of 
NOLINTBEGIN/END blocks (authored by salman-javed-nz).

Changed prior to commit:
  https://reviews.llvm.org/D116085?vs=402818&id=403196#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116085

Files:
  clang-tools-extra/clang-tidy/CMakeLists.txt
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
  clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h
  clang-tools-extra/clangd/ParsedAST.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/1st-translation-unit.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/2nd-translation-unit.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-LIFO.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-all-end-glob.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-all-end-specific.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-glob-end-all.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-glob-end-specific.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-global-end-specific.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-multiple-end-single.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-single-end-multiple.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-all.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-glob.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-global.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-multiple-TUs.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-typo-in-check-name.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
@@ -58,30 +58,24 @@
 // NOLINTEND(some-other-check)
 // NOLINTEND(google-explicit-constructor)
 
-// NOLINTBEGIN(google-explicit-constructor)
-// NOLINTBEGIN(some-other-check)
-class C7 { C7(int i); };
-// NOLINTEND(google-explicit-constructor)
-// NOLINTEND(some-other-check)
-
 // NOLINTBEGIN(google-explicit-constructor)
 // NOLINTBEGIN
-class C8 { C8(int i); };
+class C7 { C7(int i); };
 // NOLINTEND
 // NOLINTEND(google-explicit-constructor)
 
 // NOLINTBEGIN
 // NOLINTBEGIN(google-explicit-constructor)
-class C9 { C9(int i); };
+class C8 { C8(int i); };
 // NOLINTEND(google-explicit-constructor)
 // NOLINTEND
 
 // NOLINTBEGIN(not-closed-bracket-is-treated-as-skip-all
-class C10 { C10(int i); };
+class C9 { C9(int i); };
 // NOLINTEND(not-closed-bracket-is-treated-as-skip-all
 
 // NOLINTBEGIN without-brackets-skip-all, another-check
-class C11 { C11(int i); };
+class C10 { C10(int i); };
 // NOLINTEND without-brackets-skip-all, another-check
 
 #define MACRO(X) class X { X(int i); };
@@ -114,28 +108,28 @@
 MACRO_NO_LINT_INSIDE_MACRO
 
 // NOLINTBEGIN(google*)
-class C12 { C12(int i); };
+class C11 { C11(int i); };
 // NOLINTEND(google*)
 
 // NOLINTBEGIN(*explicit-constructor)
-class C15 { C15(int i); };
+class C12 { C12(int i); };
 // NOLINTEND(*explicit-constructor)
 
 // NOLINTBEGIN(*explicit*)
-class C16 { C16(int i); };
+class C13 { C13(int i); };
 // NOLINTEND(*explicit*)
 
 // NOLINTBEGIN(-explicit-constructor)
-class C17 { C17(int x); };
+class C14 { C14(int x); };
 // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: single-argument constructors must be marked explicit
 // NOLINTEND(-explicit-constructor)
 
 // NOLINTBEGIN(google*,-google*)
-class C18 { C18(int x); };
+class C15 { C15(int x); };
 // NOLINTEND(google*,-google*)
 
 // NOLINTBEGIN(*,-google*)
-class C19 { C19(int x); };
+class C16 { C16(int x); };
 // NOLINTEND(*,-google*)
 
 int array1[10];
@@ -154,4 +148,4 @@
 int array4[10];
 // NOLINTEND(*-avoid-c-arrays)
 
-// CHECK-MESSAGES: Suppressed 27 warnings (27 NOLINT).
+// CHECK-MESSAGES: Suppressed 26 warnings (26 NOLINT).
Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-multiple-TUs.cpp
==

[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

In D116085#3272200 , @nemanjai wrote:

> This is causing buildbot failures with `-Werror` (i.e. 
> https://lab.llvm.org/buildbot/#/builders/57/builds/14322/steps/5/logs/stdio). 
> Please fix or revert.

Thanks for the heads up. I have reverted in 
8e29d19b8d29db1ad60af3a8921aad7bbfc24435 
 while I 
investigate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116085

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

[PATCH] D118519: [clang-tidy] Organize the release notes a little better

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:221-227
 Removed checks
 ^^
 
 Improvements to include-fixer
 -
 
 The improvements are...

Small nit, not about this patch, but the documentation release process as a 
whole.

Clang 13 got released with a lot of these placeholders still in the 
documentation.
https://releases.llvm.org/13.0.0/tools/clang/tools/extra/docs/ReleaseNotes.html
They were never tidied up before release.

What could be done to ensure that this doesn't happen again? Perhaps keep the 
placeholders in the file but in a commented-out state?

I'm aware of https://llvm.org/docs/HowToReleaseLLVM.html#update-documentation 
but in this case I guess it wasn't enough to capture this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118519

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

[PATCH] D118104: Make run-clang-tidy.py print the configured checks correctly

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

Do you reckon it's worth expanding the test: 
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/clang-tidy/infrastructure/run-clang-tidy.cpp
 to look at this issue?
There isn't much testing of the run-clang-tidy.py at the moment, but it doesn't 
have to always remain that way.


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

https://reviews.llvm.org/D118104

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

[PATCH] D120196: [clang-tidy][NFC] Remove Tristate from CachedGlobList

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz accepted this revision.
salman-javed-nz added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120196

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