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

[Kadir Cetinkaya via Phabricator via cfe-commits]

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