[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 inline comments.


================
Comment at: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp:6
+
+// NOLINTEND
+class B1 { B1(int i); };
----------------
salman-javed-nz wrote:
> aaron.ballman wrote:
> > Do you think this should be diagnosed as a sign of user confusion with the markings?
> For a stray `NOLINTEND` like this one, I don't think so. The original warning is still raised, so I see this as clang-tidy failing safe. The user is forced to fix their mistake before the warning goes away.
> 
> The consequences are of the same severity as misusing the existing `NOLINT` and `NOLINTNEXTLINE` markers, e.g. putting `NOLINT` on the wrong line, or adding a blank line after `NOLINTNEXTLINE`.
Hmm, I'm not yet convinced we don't want to diagnose this situation. I agree that the behavior of *other* diagnostics is good (the user still gets those diagnostics because no range has been suppressed). But it seems like the programmer made a mistake if they don't balance the begin and end markers. I don't think this causes major issues, but I think the code is a bit harder to read because someone who spots the end marker may try looking for the begin marker that doesn't exist.

I suppose there's a small chance that a stray END may surprise users for more than just code readability -- consider a file with a stray end marker where the user wants to lazily suppress the end file by putting `NOLINTBEGIN` at the head of the file and `NOLINTEND` at the end of the file -- the stray `NOLINTEND` in the middle interrupts the full range.


================
Comment at: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp:86
+
+// NOLINTBEGIN
+class H1 { H1(int i); };
----------------
salman-javed-nz wrote:
> aaron.ballman wrote:
> > Should this be diagnosed as user confusion?
> > 
> > My concern in both of these cases isn't so much that someone writes this intentionally, but that one of the begin/end pair gets removed accidentally when refactoring. Helping the user to identify *where* the unmatched delimiters are seems like it could be user-friendly behavior.
> The consequences of this one are higher, as there is the potential to suppress warnings unintentionally and allow clang-tidy rule violations to go undetected. I agree that something more could be done here.
> 
> I can think of two improvements:
> 
> 1. In `LineIsMarkedWithNOLINT()`, when a `NOLINTBEGIN` is found, only return true if the corresponding `NOLINTEND` is found as well. Raise the original warning if the `NOLINTEND` is omitted.
> 
> 2. Raise an additional warning regarding the unmatched pair of delimiters. Some guidance on how to implement it would be appreciated. In the call stack of the `LineIsMarkedWithNOLINT()` function, I can't see any exposed functionality to generate new diagnostics on the fly. Would a new clang-tidy check be the place to implement this?
That's a good question -- I don't know that I would expect `LineIsMarkedWithNOLINT()` to generate a diagnostic, but it's the obvious place for checking the validity of the markers. Naively, I would not expect to have to run a linter to check my lint markings, I would expect the linting tool to do that for me.

Would it make sense for `shouldSuppressDiagnostic()` to take a container of diagnostics generated (so `LineIsMarkedWithNOLINT()` has a place to store diagnostics), and `ClangTidyDiagnosticConsumer::HandleDiagnostic()` then checks whether the container is empty and if not, emits the additional diagnostics from there?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560