salman-javed-nz added a comment. In D108560#3012755 <https://reviews.llvm.org/D108560#3012755>, @aaron.ballman wrote:
> 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. See new test for this scenario in `test\clang-tidy\infrastructure\nolintbeginend-mismatched-check-names.cpp`. Diagnostics are generated for both the `NOLINTBEGIN(check)` with no end and the `NOLINTEND(other-check)` with no begin. > My concern here is mostly with catching typos where the user types `check` in > the first and `chekc` in the second See new test in `test\clang-tidy\infrastructure\nolintbeginend-typo-in-check-name.cpp`. > Relatedly, I think this construct is perhaps fine: > > // NOLINTBEGIN(check) > // NOLINTEND(*) > > because the end "covers" the begin. That was my initial feeling too. However, after doing a bit more thinking, I feel that this construct causes more problems than it's worth. Consider the following example: // NOLINTBEGIN(check-A) <code that violates check-A> // NOLINTEND(*) This seems OK, but consider what happens when we add a 4th line: // NOLINTBEGIN(check-A) <code that violates check-A> // NOLINTEND(*) <code that violates check-B> ...this generates a `NOLINTEND without a previous NOLINTBEGIN` diagnostic on line 3. From `check-B`'s perspective, it has been `END`ed on line 3 but there was no `BEGIN(check-B)` or `BEGIN(*)` on lines 1-2. Which `NOLINT(BEGIN/END)` comments are acceptable on a given line depends on which check you're evaluating at a given moment. This problem goes away if we don't allow the check-specific `NOLINT`s and the "all checks" `NOLINT`s to be mixed and matched: Example 1: // NOLINTBEGIN(check-A) <code that violates check-A> // NOLINTEND(check-A) <code that violates check-B> Example 2: // NOLINTBEGIN <code that violates check-A> // NOLINTEND <code that violates check-B> Let's look at an example where auto-generated code with its own `NOLINT(BEGIN/END)`s is embedded in another file: // NOLINTBEGIN(check-A) <code that violates check-A> <code that violates check-A> /* * Place-holder for auto-generated code * */ <code that violates check-A> // NOLINTEND(check-A) // ... // ... If the auto-generated code is as follows: // NOLINTBEGIN(check-B) <code that violates check-B> <code that violates check-B> // NOLINTEND Then the complete file is: // NOLINTBEGIN(check-A) <code that violates check-A> <code that violates check-A> // NOLINTBEGIN(check-B) <code that violates check-B> <code that violates check-B> // NOLINTEND <code that violates check-A> // NOLINTEND(check-A) // ... // ... The `NOLINTEND` in the autogenerated code ends `check-B` as well as the parent file's `check-A`, against the user's intention. Clang-Tidy can't offer any helpful advice without mind-reading. > 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. The same rule as above, //don't mix and match check-specific `NOLINT`s with "all checks" `NOLINT`s//, should apply here. Also, in your example, you have begun all checks (`check`, `check2`, `check3` ... `checkN`) but only ended one of them. The remaining checks are still awaiting a `NOLINTEND` comment of some sort. I say all this in the interest of keeping the Clang-Tidy code simple, and reducing the amount of weird behavior that is possible (due to mistakes, lack of knowledge, or "creative" use of the feature). I rather this feature be strict and limited in scope, rather than flexible enough to be used in unforeseen error-prone ways. Perhaps this feature can be extended in the future (if need be) after it gets some use and user feedback comes in? 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
