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

Kadir Cetinkaya via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Dec 22 10:35:16 PST 2021

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?

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list