[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<NoLintBlockToken>
+collectNoLintBlocks(const ClangTidyContext &Context, const SourceManager &SM,
+                    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<size_t, size_t> 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