aaron.ballman added a comment.
Thanks, I think this is getting close! There are two more test cases that I
think are interesting (and should cause any issues, hopefully):
// NOLINTEND
// CHECK-MESSAGES: for diagnostic on the previous line
and
// CHECK-MESSAGES: for diagnostic on the subsequent line
// NOLINTBEGIN
as the only contents in the file. The idea is that we want to check that
searching for a "begin" when seeing an "end" at the top of the file doesn't do
anything silly like try to read off the start of the file, and similar for
"end" when seeing a "begin" at the end of a file.
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:208
+ SourceManager &SM = DiagEngine->getSourceManager();
+ auto File = SM.getFileManager().getFile(Error.Message.FilePath);
+ FileID ID = SM.getOrCreateFileID(*File, SrcMgr::C_User);
----------------
This helps the reader of the code to understand that `*File` below is doing
something special (that includes an assert check, which is great).
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:367
+ Context.getCurrentBuildDirectory(), false);
+ Error.Message = tooling::DiagnosticMessage("mismatched pair of NOLINTBEGIN/"
+ "NOLINTEND comments",
----------------
This message works, but I think it'd be better if we split it into two messages:
`unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINTEND' comment`
`unmatched 'NOLINTEND' comment without a previous 'NOLINTBEGIN' comment`
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:381-387
+ // Check if a new block is being started.
+ if (isNOLINTFound("NOLINTBEGIN", CheckName, Line, &NolintIndex)) {
+ NolintBegins.emplace_back(LinesLoc.getLocWithOffset(NolintIndex));
+ }
+ // Check if the previous block is being closed.
+ else if (isNOLINTFound("NOLINTEND", CheckName, Line, &NolintIndex)) {
+ if (!NolintBegins.empty()) {
----------------
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:390
+ } else {
+ // Trying to close a non-existant block. Return a diagnostic about this
+ // misuse that can be displayed along with the original clang-tidy
check
----------------
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:509-510
bool AllowIO) {
+ std::vector<ClangTidyError> Unused;
+ return shouldSuppressDiagnostic(DiagLevel, Info, Context, Unused, AllowIO);
+}
----------------
Should we assert that `Unused` is empty before returning?
================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:237
const Diagnostic &Info, ClangTidyContext
&Context,
+ std::vector<ClangTidyError> &SuppressionErrors,
bool AllowIO = true);
----------------
Might be better as a `SmallVectorImpl` as this should almost always be a
container of 0 or 1 elements.
================
Comment at:
clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-without-end.cpp:6-8
+// Note: the expected output has been split over several lines so that
clang-tidy
+// does not see the "no lint" suppression comment and mistakenly assume
it
+// is meant for itself.
----------------
THANK YOU for these comments! :-)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D108560/new/
https://reviews.llvm.org/D108560
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
