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