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

Thu Dec 2 14:06:01 PST 2021

salman-javed-nz added a comment.

In D108560#3168006 <https://reviews.llvm.org/D108560#3168006>, @ymandel wrote:

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

If this is the approach we take, I prefer the logic to be switched around: the NOLINT block feature should be enabled by default. Once (the worst of) this performance issue is resolved, it would be nice from a usability perspective if the user did not have to explicitly opt in to use the feature.

> 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.

Yes, that sounds better to me.

> 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).

I'll do my best to make this feature as efficient as possible. There will always be some performance impact - after all, we are asking clang-tidy to do more than it did before. I wonder how much the execution time increases when a new clang-tidy check is added.
I don't have the answer to what's the right balance between performance and functionality. The NOLINT block feature has been something people have been asking for for a long time. How much performance hit are they willing to take to make suppressing clang-tidy warnings a bit easier?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108560