[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks
Salman Javed via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 22 12:24:37 PST 2021
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(
return diag(Error.DiagnosticName, Loc, Error.Message.Message,
> 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
+ // 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?
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()".
> 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:
Error: NOLINTEND does not match previous NOLINTBEGIN
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
More information about the cfe-commits