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


================
Comment at: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp:6
+
+// NOLINTEND
+class B1 { B1(int i); };
----------------
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`.


================
Comment at: clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp:86
+
+// NOLINTBEGIN
+class H1 { H1(int i); };
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560