[PATCH] D108560: [clang-tidy] Add support for NOLINTBEGIN ... NOLINTEND comments to suppress clang-tidy warnings over multiple lines

[Aaron Ballman via Phabricator via cfe-commits]

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