aaron.ballman added a comment.

Sorry for not thinking of this sooner, but there is another diagnostic we might 
want to consider.

  // NOLINTBEGIN(check)
  // NOLINTEND(other-check)

where the file does not contain a `NOLINTEND(check)` comment anywhere. In this 
case, the markers are not actually matched, so it's a `NOLINTBEGIN(check)` 
comment with no end and a `NOLINTEND(other-check)` comment with no begin. That 
seems like user confusion we'd also want to diagnose, but it also could be 
tricky because you really want to add diagnostic arguments for the check name. 
My concern here is mostly with catching typos where the user types `check` in 
the first and `chekc` in the second, so if we wanted to use the edit distance 
between what was provided and the list of check names to provide a fix-it, that 
would be very handy (and also probably difficult to implement so I definitely 
don't insist on a fix-it).

Relatedly, I think this construct is perhaps fine:

  // NOLINTBEGIN(check)
  // NOLINTEND(*)

because the end "covers" the begin. I'm a bit less clear on this though:

  // NOLINTBEGIN(*)
  // NOLINTEND(check)

because the begin is not fully covered by the end. However, I'm a bit less 
clear on what should or should not be diagnosed here, so if we wanted to leave 
that for follow-up work, that'd be fine.


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

Reply via email to