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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:214
+  std::string CheckName = getCheckName(Info.getID());
+  return NoLintHandler.shouldSuppress(DiagLevel, Info, CheckName, NoLintErrors);
+}
----------------
we don't seem to be passing allowio/enablenolintblocks ?


================
Comment at: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp:89
+// as parsed from the file's character contents.
+class NoLintToken {
+public:
----------------
nit: Any reason for not making this class move-only ? everytime we copy, we are creating globs from scratch and losing the cache. i think it should be fine if we just did emplace_back in all places and moved tokens around rather than taking copies/pointers.


================
Comment at: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp:133
+static SmallVector<NoLintToken> getNoLints(StringRef Buffer) {
+  static const StringRef NOLINT = "NOLINT";
+  SmallVector<NoLintToken> NoLints;
----------------
nit: `static constexpr llvm::StringLiteral`


================
Comment at: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp:141
+    if (NoLintPos == StringRef::npos)
+      return NoLints; // Buffer exhausted
+
----------------
nit: `break` instead?


================
Comment at: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp:156
+    // Get checks, if specified.
+    const auto Checks = [&]() -> Optional<std::string> {
+      // Opening bracket:
----------------
nit: maybe drop the lambda? direct version has similar/less indentation.
```
llvm::Optional<std::string> Checks;
if(Pos < Buffer.size() && Buffer[Pos] == '(') {
  auto ClosingBracket = Buffer.find_first_of("\n)", ++Pos);
  if (ClosingBracket != Buffer.npos && Buffer[ClosingBracket] == ')')
   Checks = Buffer.slice(Pos, ClosingBracket).str();
}
```


================
Comment at: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp:170
+
+    NoLints.emplace_back(NoLintToken(*NoLintType, NoLintPos, Checks));
+  }
----------------
nit: you can drop the `NoLintToken`


================
Comment at: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp:427
+NoLintDirectiveHandler::NoLintDirectiveHandler()
+    : Impl(std::make_unique<class Impl>()) {}
+
----------------
no need for `class` here.


================
Comment at: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h:41
+                      llvm::SmallVectorImpl<tooling::Diagnostic> &NoLintErrors,
+                      bool AllowIO = true, bool EnableNoLintBlocks = true);
+
----------------
i feel like this is still an implementation detail of the ClangTidyContext, we have it publicly available just to reduce amount of extra code inside a single source file. so i think it makes sense for these parameters to not have defaults (as the one in ClangTidyContext already has).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116085