[PATCH] D108560: [clang-tidy] Add support for `NOLINTBEGIN` ... `NOLINTEND` comments

Thu Dec 2 13:08:42 PST 2021

ymandel added a comment.

Thanks, all, for the prompt response to these concerns!!

In D108560#3167847 <https://reviews.llvm.org/D108560#3167847>, @salman-javed-nz wrote:

> Thanks for the investigation. Just exploring the options...
>
> With regards to reverting it, is the cat out of the bag? I already see some usage of the NOLINTBEGIN feature in other projects.
> There's also a NOLINT check globbing feature that builds on top of this, so it's not as simple as just backing out one commit.

Agreed. I think something along the lines of https://reviews.llvm.org/D114981 is more realistic -- turning it off without backing out the code.

> I can have a go at coming up with a solution where it does caching of the NOLINT marker positions when a diagnostic is generated in a file. That would probably help a lot with the performance. It wouldn't be too complicated to implement either. Would you be happy with something like that?

Yes, that sounds good!

> If the current code stayed in the main repo, how long could you afford to wait for me to provide a patch?

If there's something we could do ASAP, we'd prefer that to waiting given that its having measurable impact already.  I wonder if we change my patch so that it leaves the feature enabled by default, it would still allow us to modify internal tools to disable the feature in the interim.

Also, I'm concerned that even an optimized implementation of this might have noticable (if much smaller) impact, since it will be adding an O(n) pass to clang-tidy. Compared with the cost of building the AST this may be trivial, but still. So, the parameter I added to the constructor might be valuable long term to allow clients of the class to disable this feature, even if we don't want to expose it as a CLI flag (for the reasons Aaron mentioned above).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560