[PATCH] D114317: [clang-tidy][WIP] Do not run perfect alias checks

Thu Dec 16 06:11:58 PST 2021

aaron.ballman added a reviewer: alexfh.
aaron.ballman added a subscriber: alexfh.
aaron.ballman added a comment.

In D114317#3169067 <https://reviews.llvm.org/D114317#3169067>, @carlosgalvezp wrote:

> Kind ping to reviewers, I mostly want to know if you agree with the general direction so I can go ahead and implement the rest of the patch. It's a big change (replace all check registrations with the macro) so I'd like to avoid extra work if you are strongly against it.
>
> @aaron.ballman I understand you are super busy and don't want to take any more time than needed from you. I'd just love to hear your thoughts about the two missing points that we discussed in the RFC. Nobody else seems to have raised similar concerns about it.
>
> Really appreciate your time!

Sorry about the lack of response! Taking (effectively) two weeks off put me pretty far behind on reviews (I'm down to "only" ~30 reviews in my queue as of this morning), and I head out for another vacation starting tomorrow until the end of the year. So please anticipate further delays from me, sorry!

> It would be desirable to still keep the diagnostic/fixit for the alias checks. I don't know how to implement this in practice. It would require each check to call the diag() function in a loop for all alias checks. This would be very disruptive for check developers. I cannot implement this in the ClangTidyCheck class because the diag() function returns an object, so I can't put a loop there.

I'd like to make sure I'm on the same page as you. What you mean here is that you want the diagnostic to be something like: `error: forgot to frobble the quux [bugprone-frobble-quux, cert-quux45-cpp]` instead of error: forgot to frobble the quux [bugprone-frobble-quux]`? Or do you mean something else?

> Ideally, it would be nice to show "CACHED - 0 seconds" or "SKIPPED - 0 seconds" in the profiling output (currently, the alias check just doesn't show up). Again, I don't know how to implement this feature in a non-intrusive way that makes sense.

That would be handy, but I think is totally reasonable for a follow-up.

> I see, thanks for the input! I took configuration into account due to the concerns raised in previous patch attempts and in the RFC. I believe both use cases are valid so it makes sense to me to support both as independent opt-in flags. What do you think, @aaron.ballman ?

Eh, I'm less convinced of this, but I'm also not certain I'm strongly opposed (mostly weakly opposed at this point). Aliases with different configuration options are positioned to users as being separate checks. For example, we could have one check that decides to convert all punctuators it can to be alternate tokens (`and` instead of `&&`, etc) and there could be an alias check that converts all alternative tokens back into punctuators but is implemented as an alias. The fact that it's an alias is an implementation detail that should not matter to the user, and being able to disable *all* checks regardless of their configuration strikes me as making these kinds of situations confusing to users and I think it closes a development door we don't really want to close. I'd want to have buy-in from @alexfh as code owner for that option. However, I think it'd be good to ensure that the design can support such an option in the future (at least, support as much as reasonable) so we can do it as follow-up work without extra effort.

================
Comment at: clang-tools-extra/clang-tidy/ClangTidyModule.h:25-26

+#define CLANG_TIDY_REGISTER_CHECK(Factory, CheckType, CheckName)               \
+  Factory.registerCheck<CheckType>(CheckName, #CheckType)
+
----------------
I'm not 100% sold that this macro carries its weight. I almost think the registration code is easier to read without it, but not enough to suggest a change yet. Curious if others have feelings here.

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

https://reviews.llvm.org/D114317