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