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

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz marked 5 inline comments as done.
salman-javed-nz added inline comments.


================
Comment at: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp:6
+
+// NOLINTEND
+class B1 { B1(int i); };
----------------
aaron.ballman wrote:
> 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.
I see your point. Clang-Tidy should alert the user about any stray `NOLINTBEGIN` or `NOLINTEND` markers it sees, not leave it up to the user to find them themselves. Not diagnosing this is only going to create more headache for the user in the long run.


================
Comment at: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp:86
+
+// NOLINTBEGIN
+class H1 { H1(int i); };
----------------
aaron.ballman wrote:
> 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?
Thank you for the suggestion. That is exactly what I have implemented in my latest patch. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560