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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
