[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks
Carlos Galvez via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 21 05:05:14 PST 2022
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
More information about the cfe-commits
mailing list