[PATCH] D69560: [clang-tidy] Add 'experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type' check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D69560#2486806 <https://reviews.llvm.org/D69560#2486806>, @whisperity wrote:

> I was just about to write an issue to the CoreGuidelines project, but I was @vingeldal has posted about "relatedness" before me: https://github.com/isocpp/CppCoreGuidelines/issues/1579. It was in the list of //closed// issues.
>
> Herb Sutter has changed the rule's title to //"Avoid adjacent parameters of the same type when changing the argument order would change meaning"//. Now the phrase `related` has been removed from the title, and the body of the rule does not contain any mention to this phrase either.
>
> //`change of meaning`// is still hard to prove from a Tidy standpoint (or could be incomputable to prove in the first place...), but a concept that can be grasped with much less mental effort.
>
> The guideline itself still says that the //Enforcement// is to simply warn if two parameters have the same type. We are doing much more here already with making the Tidy output much more user friendly and less bloatful.

Thank you for continuing to work on this and working with the rule authors. I agree that "changes meaning" would be really hard to gauge within clang-tidy (but is more precise for a human auditing the code). In theory, we could take the command line options and use them to compile a function to LLVM IR, swap two adjacent parameter identifiers and recompile, then see if the IR is the same or not. But my gut feeling is that this would make the diagnostic behavior very mysterious to users (you change command line flags from -O2 to -O0 and suddenly you get different diagnostics because the optimizations are different) and is not something we'd want to do.

> In addition to all this, and tracking back to the talk about default configuration... @aaron.ballman, I have some questions. I've seen that Tidy now supports "check aliases". Do you think moving forward in a way that we rename this check > and put it into another group (maybe calling it bugprone-adjacent-parameters-of-same-type) with more sensible defaults and offering a version of the check under cppcoreguidelines- with the defaults more matching the guideline rule > would be a good way forward, eventually? Implicit conversions seem to be a real edge of the analysis (hence why I pinned my paper about that problem, also, this was the novelty, the rest of the mixability analysis was done decades
> ago), and seem to hurt more than simple type equality. However, the guideline is cautious about only warning for same type, and considers const T* and T* already distinct.

Sorry, I missed your question from earlier. I think using a check alias with different option defaults is a workable approach. We may have to live with the fact that C++ Core Guideline check is really chatty and only useful for programs in very specific situations (and that's fine, we do that for other check guidelines sometimes). Having a check with better default options for our users is a good compromise.


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

https://reviews.llvm.org/D69560