[PATCH] D116085: [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

[Salman Javed via Phabricator via cfe-commits]

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:
```
// NOLINTBEGIN()
// NOLINTEND(some-check)
   ^
   Error: NOLINTEND does not match previous NOLINTBEGIN
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116085/new/

https://reviews.llvm.org/D116085