[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] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai 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. The failure is:
>
>   
> .../llvm-project/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp:204:38:
>  error: moving a temporary object prevents copy elision 
> [-Werror,-Wpessimizing-move]
>   CompletedBlocks.emplace_back(std::move(Stack.pop_back_val()), 
> NoLint);

Sorry, I didn't realize you already reverted as there was no mention in the 
revert of the original commit.


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

[Nemanja Ivanovic via Phabricator via cfe-commits]

nemanjai added a comment.

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. The failure is:

  
.../llvm-project/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp:204:38:
 error: moving a temporary object prevents copy elision 
[-Werror,-Wpessimizing-move]
  CompletedBlocks.emplace_back(std::move(Stack.pop_back_val()), NoLint);


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]

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=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

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp accepted this revision.
carlosgalvezp added a comment.

Looks great!


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 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
+++ 

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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

thanks, lgtm!




Comment at: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp:187
+static SmallVector
+formNoLintBlocks(SmallVector ,
+ SmallVectorImpl ) {

let's drop the `&` here to make sure caller does an explicit `std::move`. since 
the helper is actually consuming the tokens inside the vector and in theory it 
shouldn't be used afterwards.


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 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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:214
+  std::string CheckName = getCheckName(Info.getID());
+  return NoLintHandler.shouldSuppress(DiagLevel, Info, CheckName, 
NoLintErrors);
+}

we don't seem to be passing allowio/enablenolintblocks ?



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

nit: Any reason for not making this class move-only ? everytime we copy, we are 
creating globs from scratch and losing the cache. i think it should be fine if 
we just did emplace_back in all places and moved tokens around rather than 
taking copies/pointers.



Comment at: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp:133
+static SmallVector getNoLints(StringRef Buffer) {
+  static const StringRef NOLINT = "NOLINT";
+  SmallVector NoLints;

nit: `static constexpr llvm::StringLiteral`



Comment at: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp:141
+if (NoLintPos == StringRef::npos)
+  return NoLints; // Buffer exhausted
+

nit: `break` instead?



Comment at: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp:156
+// Get checks, if specified.
+const auto Checks = [&]() -> Optional {
+  // Opening bracket:

nit: maybe drop the lambda? direct version has similar/less indentation.
```
llvm::Optional Checks;
if(Pos < Buffer.size() && Buffer[Pos] == '(') {
  auto ClosingBracket = Buffer.find_first_of("\n)", ++Pos);
  if (ClosingBracket != Buffer.npos && Buffer[ClosingBracket] == ')')
   Checks = Buffer.slice(Pos, ClosingBracket).str();
}
```



Comment at: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp:170
+
+NoLints.emplace_back(NoLintToken(*NoLintType, NoLintPos, Checks));
+  }

nit: you can drop the `NoLintToken`



Comment at: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp:427
+NoLintDirectiveHandler::NoLintDirectiveHandler()
+: Impl(std::make_unique()) {}
+

no need for `class` here.



Comment at: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h:41
+  llvm::SmallVectorImpl ,
+  bool AllowIO = true, bool EnableNoLintBlocks = true);
+

i feel like this is still an implementation detail of the ClangTidyContext, we 
have it publicly available just to reduce amount of extra code inside a single 
source file. so i think it makes sense for these parameters to not have 
defaults (as the one in ClangTidyContext already has).


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] 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 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 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 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 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 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

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:347
+NoLint.SpecifiesChecks = true;
+  }
+}

Nit: tab with spaces



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:390
+static SmallVector
+collectNoLintBlocks(const ClangTidyContext , const SourceManager ,
+StringRef Buffer, SourceLocation BufferLoc,

As a non-native English speaker, I appreciate the name change :) 



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:422
+Errors.emplace_back(Error);
+  }
+

Nit: tab with spaces



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;

Don't we usually use `SourceLocation` objects for this?



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:480
+if (isNoLintFound(PrevLine, NoLintToken::Type::NoLintNextLine, CheckName))
+  return true;
+  }

Tab with spaces



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:563
+return true;
+  GlobList Globs(Checks, /*KeepNegativeGlobs=*/false);
+  return Globs.contains(CheckName);

kadircet wrote:
> this creates new regexes at each call, what about building the glob once in 
> constructor and storing that instead of `Checks` ?
Maybe good to keep the original comment about why negative globs are excluded?



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:563
+return true;
+  GlobList Globs(Checks, /*KeepNegativeGlobs=*/false);
+  return Globs.contains(CheckName);

carlosgalvezp wrote:
> kadircet wrote:
> > this creates new regexes at each call, what about building the glob once in 
> > constructor and storing that instead of `Checks` ?
> Maybe good to keep the original comment about why negative globs are excluded?
+1. Also, in case it helps, you can use `CachedGlobList`.


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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/1st-translation-unit.cpp:1
+// NOLINTBEGIN
+class A { A(int i); };

kadircet wrote:
> no run lines or anything here (and the following file)
oops, nevermind this one. I've seen the testing entry down below but forgot to 
delete the comment.


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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

here are some more comments, can't say I've been able to go through all of the 
patch yet, unfortunately it's considerably big in size. it would be great to 
get rid of some of those extra code by just dropping accessors/classes when not 
needed.




Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:161
 std::unique_ptr OptionsProvider,
-bool AllowEnablingAnalyzerAlphaCheckers)
+bool AllowEnablingAnalyzerAlphaCheckers, bool AllowIO,
+bool EnableNoLintBlocks)

i think having an `AllowIO` parameter is quite confusing here. It doesn't, and 
probably won't, control all the IO that might be done directly or transitively 
through `ClangTidyContext` as one would naturally expect.

what about just making it a parameter to `shouldSuppressDiagnostic` call ?
when IO is allowed, the suppression logic will be able to fetch the contents 
and work as expected so no problems there.
when it is not, we'll either fail to get the buffer and bail out early, or 
we'll get the buffer from memory and again everything will work as intended.
In either case the cache is never poisoned, so we can call 
`shouldSuppressDiagnostic` with different values of `AllowIO` on a single 
instance of `ClangTidyContext`.
The same applies to `EnableNoLintBlocks` too. So lets move both back into the 
parameters of `shouldSuppressDiagnostics`.



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

why are we moving these two parameters here?



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:107
 
-  DiagnosticBuilder diag(const ClangTidyError );
+  DiagnosticBuilder diag(const NoLintError );
 

can we not introduce a new error type in this patch, as it is already doing a 
good amount of changes?



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:234
 
-/// Check whether a given diagnostic should be suppressed due to the presence
-/// of a "NOLINT" suppression comment.
-/// This is exposed so that other tools that present clang-tidy diagnostics
-/// (such as clangd) can respect the same suppression rules as clang-tidy.
-/// This does not handle suppression of notes following a suppressed 
diagnostic;
-/// that is left to the caller as it requires maintaining state in between 
calls
-/// to this function.
-/// If `AllowIO` is false, the function does not attempt to read source files
-/// from disk which are not already mapped into memory; such files are treated
-/// as not containing a suppression comment.
-/// \param EnableNolintBlocks controls whether to honor NOLINTBEGIN/NOLINTEND
-/// blocks; if false, only considers line-level disabling.
-/// If suppression is not possible due to improper use of "NOLINT" comments -
-/// for example, the use of a "NOLINTBEGIN" comment that is not followed by a
-/// "NOLINTEND" comment - a diagnostic regarding the improper use is returned
-/// via the output argument `SuppressionErrors`.
-bool shouldSuppressDiagnostic(
-DiagnosticsEngine::Level DiagLevel, const Diagnostic ,
-ClangTidyContext ,
-SmallVectorImpl , bool AllowIO = true,
-bool EnableNolintBlocks = true);
+  std::unique_ptr NoLintHandler;
+};

no need to make this dependency weak, let's just depend on the public header 
and make this `NoLintPragmaHandler NoLintHandler`.



Comment at: clang-tools-extra/clang-tidy/NoLintPragmaHandler.cpp:37
+namespace tidy {
+
+//===--===//

place class/struct definitions in this file inside an anonymous namespace.



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

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).



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

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)?



Comment at: clang-tools-extra/clang-tidy/NoLintPragmaHandler.cpp:90
+  size_t Pos;
+  Optional Checks;
+  mutable std::unique_ptr ChecksGlob;

this deserves a comment about 

[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
===
--- 

[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 ,
+SmallVectorImpl );
+

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

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

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 :)




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

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`?



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

Why not `SmallVector`? Sounds like the `Impl` is some "private" implementation?



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

Why not `SmallVector`?


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.

Friendly ping.

Would be good to get these performance improvements into trunk soon, so that 
we're not prolonging the time that people are putting up with the current slow 
implementation. Also, I believe that LLVM 14.0.0 will be up for a release 
candidate soon, and all going well, the NOLINTBEGIN feature would be debuting 
in it. It would be nice if the feature shipped in this faster incarnation. It 
would be a pity if users forgo using this feature because they are put off by 
the slowness of it.

@kadircet as the clangd expert, I'm especially interested in your thoughts on 
this patch, as clangd is sensitive to clang-tidy's performance. I compiled 
clangd and tested it with the vscode-clangd extension. Everything worked as 
expected when moving lines around, applying fixits, and switching tabs. 
Performance looked OK to me too, but I haven't used clangd all that much so I 
don't have a baseline on what is normal performance.

Thanks a lot.


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 398087.
salman-javed-nz added a comment.

Comments


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/NoLintPragmaHandler.cpp
  clang-tools-extra/clang-tidy/NoLintPragmaHandler.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/test/clang-tidy/infrastructure/nolintbeginend-typo-in-check-name.cpp
@@ -11,3 +11,6 

[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 398048.
salman-javed-nz added a comment.
Herald added subscribers: usaxena95, arphaman, mgorny.

Updated according to review comments

- Move implementation to a CPP file. Use a PIMPL for access.


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/NoLintPragmaHandler.cpp
  clang-tools-extra/clang-tidy/NoLintPragmaHandler.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 added a comment.

Thanks for taking a look. I will update the diff to address your comments. Have 
a good new years break.




Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:197
+  SourceLocation Loc = FileStartLoc.getLocWithOffset(
+  static_cast(Error.Message.FileOffset));
   return diag(Error.DiagnosticName, Loc, Error.Message.Message,

kadircet wrote:
> this change looks unrelated to the current patch.
I suppose it is. It's fixing a lint issue in a function used by the NOLINTBEGIN 
functionality, but not needed to implement the caching functionality. I don't 
have strong feelings about whether this change is bundled in this patch or not.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:602
EnableNolintBlocks)) {
-++Context.Stats.ErrorsIgnoredNOLINT;
+// Even though this diagnostic is suppressed, there may be other issues 
with
+// the file that need raising, such as unclosed NOLINT(BEGIN/END) blocks.

kadircet wrote:
> looks like an unrelated change, maybe move to a separate patch?
This is needed for this patch. The diagnostic in question may be suppressed 
(`shouldSuppressDiagnostic() = true`) but the cache generation could return 
errors to do with badly formed NOLINTBEGIN blocks for other checks in the file. 



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:76
+  // NOTE: at first glance, *it may seem* that this bool is redundant when the
+  // `Checks.empty()` method already exists.
+  // However, `Checks.empty()` cannot differentiate "NOLINT" from "NOLINT()".

kadircet wrote:
> similar to above is `NOLINT()` useful at all? maybe we should just not 
> generate such tokens?
`NOLINT()` is a pretty silly statement, but it is specially mentioned in the 
clang-tidy documentation.
I didn't want the parser to just swallow `NOLINT()`s as if they were never 
there, because I still want to pick up errors like:
```
// NOLINTBEGIN()
// NOLINTEND(some-check)
   ^
   Error: NOLINTEND does not match previous NOLINTBEGIN
```


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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

some drive-by comments, i'll be AFK for a while though. so please don't block 
this on my approval if people disagree.




Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:197
+  SourceLocation Loc = FileStartLoc.getLocWithOffset(
+  static_cast(Error.Message.FileOffset));
   return diag(Error.DiagnosticName, Loc, Error.Message.Message,

this change looks unrelated to the current patch.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:455
 static bool
 lineIsMarkedWithNOLINT(const ClangTidyContext ,
SmallVectorImpl ,

as mentioned above, if we were to make this 
`NoLintPragmaHandler::isLineWithinNoLint` in which `NoLintPragmaHandler` owns 
the cache of `NoLintTokens` per `FileID`. We can just first populate the cache 
(or use the already cached tokens) and then iterate over them here to see if 
any of them suppresses diagnostic at hand.

I don't think it's worth optimizing the iteration over tokens, as we won't have 
more than a handful of nolint tokens in a single file. but if that assumption 
turned out to be wrong, i suppose we can also do a binary search over tokens 
rather than iterating over all of them in the future.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:546
+NoLintToken::Type NoLintToken::strToType(StringRef S) {
+  static const llvm::StringMap StrMapping = {
+  {"NOLINT", Type::NoLint},

nit: there's also `llvm::StringSwitch`



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:563
+return true;
+  GlobList Globs(Checks, /*KeepNegativeGlobs=*/false);
+  return Globs.contains(CheckName);

this creates new regexes at each call, what about building the glob once in 
constructor and storing that instead of `Checks` ?



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:580
+const SourceManager ) const {
+  if (!isValidPair(Begin, End, SM))
+return false;

performing this check once during construction sounds better than performing it 
for every diagnostics.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:602
EnableNolintBlocks)) {
-++Context.Stats.ErrorsIgnoredNOLINT;
+// Even though this diagnostic is suppressed, there may be other issues 
with
+// the file that need raising, such as unclosed NOLINT(BEGIN/END) blocks.

looks like an unrelated change, maybe move to a separate patch?



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:66
+struct NoLintToken {
+  enum class Type { Unknown, NoLint, NoLintNextLine, NoLintBegin, NoLintEnd };
+  Type NoLintType = Type::Unknown;

instead of having an unknown type here and making it a concern for all the 
places handling the token, can we just ignore such tokens during generation?

it'd probably imply having a constructor for the object which takes Location, 
Type and Checks as input.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:76
+  // NOTE: at first glance, *it may seem* that this bool is redundant when the
+  // `Checks.empty()` method already exists.
+  // However, `Checks.empty()` cannot differentiate "NOLINT" from "NOLINT()".

similar to above is `NOLINT()` useful at all? maybe we should just not generate 
such tokens?



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:94
+  NoLintToken Begin;
+  NoLintToken End;
+

why not just store a `SourceLocation` for the end and make sure we only 
construct valid ones?



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:236
+  const SmallVector &
+  getNoLintBlocks(FileID File, SmallVectorImpl ,
+  bool AllowIO = true) const;

what about a narrower interface? e.g. moving `shouldSuppressDiagnostics` into 
`ClangTidyContext`. It already takes the `Context` as a mutable-ref.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:265
+
+  mutable llvm::DenseMap> NoLintCache;
 };

what about just having:
```
class NoLintPragmaHandler;
std::unique_ptr NoLintHandler;
```

And making all of this an implementation detail (including the 
NoLintToken/Block structs above)?

We can then have a `NoLintPragmaHandler::isLineWithinNoLint` which performs all 
the checks while filling the cache if need be?


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

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

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

Updated the review's edit permissions. Sorry about that, @kadircet.


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

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

[Kadir Çetinkaya via cfe-commits]

Hi Salman, it looks like patch doesn't have edit access for anyone but you.
I had drafted some comments but can't hit the submit button.
I think you can go to
https://reviews.llvm.org/differential/revision/edit/116085/ and change the
`Editable By` field to `All Users`

On Tue, Dec 21, 2021 at 8:45 AM Salman Javed via Phabricator <
revi...@reviews.llvm.org> wrote:

> salman-javed-nz updated this revision to Diff 395606.
> salman-javed-nz added a comment.
>
> Remove unnecessary `llvm::` qualification.
>
>
> 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/ClangTidyDiagnosticConsumer.cpp
>   clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
>   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-typo-in-check-name.cpp
>   clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp
>
>
___
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 395606.
salman-javed-nz added a comment.

Remove unnecessary `llvm::` qualification.


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/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  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-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/test/clang-tidy/infrastructure/nolintbeginend-typo-in-check-name.cpp
@@ -11,3 +11,6 @@
 // CHECK: TBEGIN' comment without a subsequent 'NOLIN
 // CHECK: TEND' comment [clang-tidy-nolint]
 // CHECK: :[[@LINE-9]]:11: warning: single-argument constructors must be marked explicit
+// CHECK: :[[@LINE-9]]:4: error: unmatched 'NOLIN
+// CHECK: TEND' comment without a previous 'NOLIN
+// CHECK: TBEGIN' comment [clang-tidy-nolint]
Index: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-mismatched-delims.cpp