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

Thu Dec 16 08:54:44 PST 2021

aaron.ballman added a comment.

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

>> me pretty far behind on reviews
>
> Wow, that's a lot to review! I will keep it in mind and hopefully only ping you when I have a nice final design ready to go. Really appreciate your early feedback!

Happy to help and I really appreciate your patience!

>> I'd like to make sure I'm on the same page as you.
>
> Yes, exactly, from the RFC we agreed that the desired outcome of this patch would be that the same diagnostics and fixits are emitted, regardless of the alias checks actually running. I.e. this patch should not have any visible effects towards the user (other than faster runtime). The patch, as it is right now _will_ eliminate those diagnostics (since the alias checks are removed from the list), which to my understanding are not desirable. In order to implement this, the primary check needs to emit copies of the same diagnostic as if it were the alias check.

Okay, thank you for verifying! I keep reading "copies of the same diagnostic" as though you wanted:

  error: forgot to frobble the quux [bugprone-frobble-quux]
  error: forgot to frobble the quux [cert-quux45-cpp]

and going "no, I don't think that's quite what we want." :-)

If it turns out that producing output like `error: forgot to frobble the quux [bugprone-frobble-quux, cert-quux45-cpp]` is really difficult, I don't see it as being a deal breaker to only emit the primary check name. (Same goes for things like `LINT` comments naming the perfect alias vs naming the primary.)

The idea I had for supporting this was: we already register which check is the primary and all the names of the aliases, and so we don't need to really run the alias check to get its diagnostic output, we could thread some way to go from the diagnostics engine back to our registered lists of aliases, and then print the alias names manually when emitting the diagnostic for the primary. However, I've not tried this idea out and who knows what details I'm missing.

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

+#define CLANG_TIDY_REGISTER_CHECK(Factory, CheckType, CheckName)               \
+  Factory.registerCheck<CheckType>(CheckName, #CheckType)
+
----------------
carlosgalvezp wrote:
> aaron.ballman wrote:
> > 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.
> I'm not loving it either, to be honest, but I couldn't think of anything better that wouldn't cause developer friction. I've looked into RTTI (banned in LLVM) and hand-made LLVM RRTI, which I think would be quite a burden for check developers since they'd need to add a new enum to the list, implement a getClass() method, etc (IIUC).
> 
> Previous attempts as this problem created a different register function like:
> 
> ```
> Factory.registerCheck("foo-check", misc::PrimaryCheck, "misc-check")
> ```
> 
> Which I don't like much because the developer needs to spend time digging through the code and docs to see what is the name of the check it's aliasing, which is error-prone and can easily get out of sync in the future. 
> 
> With this macro, all of this is automated and always in sync. But again, if someone has some cleverer magic that would work here I'd love to bring it in :) 
> 
> Since replacing existing lines with the macro is a pretty big change, I'll wait until I get OK from reviewers to avoid spending time if in the end we go for another solution.
> With this macro, all of this is automated and always in sync.

Yup, and this is the primary reason I'm not suggesting a change yet. This solves the problem we needed to solve, it's just a bit... crufty, maybe? I dunno. I would not spend time finding a replacement yet; if someone has a great idea, we can consider it then.

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

https://reviews.llvm.org/D114317