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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
