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<SourceLocation::IntTy>(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 &Context,
SmallVectorImpl<ClangTidyError> &SuppressionErrors,
----------------
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<Type> 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 &SM) 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<NoLintBlockToken> &
+ getNoLintBlocks(FileID File, SmallVectorImpl<ClangTidyError> &Errors,
+ 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<FileID, SmallVector<NoLintBlockToken>> NoLintCache;
};
----------------
what about just having:
```
class NoLintPragmaHandler;
std::unique_ptr<NoLintPragmaHandler> 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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
