[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(
+      static_cast<SourceLocation::IntTy>(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:
// NOLINTEND(some-check)
   Error: NOLINTEND does not match previous NOLINTBEGIN

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list