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

Mon Mar 23 05:26:09 PDT 2020

aaron.ballman added a comment.

In D69560#1936153 <https://reviews.llvm.org/D69560#1936153>, @vingeldal wrote:

> In D69560#1935071 <https://reviews.llvm.org/D69560#1935071>, @whisperity wrote:
>
> > @aaron.ballman I've gone over LLVM (and a few other projects). Some general observations:
> >
> > - Length of `2` **is vile**. I understand that the C++CG rule says even lengths of 2 should be matched, but that is industrially infeasible unless one introduces such a rule incrementally to their project. Findings of length 2 are **, in general,** an order of magnitude more than... basically the rest of the findings.
> >   - On big projects, even the current "default" of "length `3`" seems to be too low. In reality, one should consider (this is likely to be out of scope for Tidy) how often these functions are called, and various other metrics on how serious an offender is.
>
>
> Not a problem since existing projects can just use the option and change to whatever level suits them best. My opinion is still that the default shouldn't be set to whatever we think is most useful for the majority of existing projects but to what reflects the actual guideline.

That's what we try to aim for, but ultimately it means we have to decide whether that means we're willing to abandon a check over it or not (and we traditionally have not abandoned checks, but instead gone with more reasonable defaults). This is a case where I would probably rather abandon the check than go with that poor of a default behavior.

I think a more productive way forward is to collaborate with the rule authors until they have a rule that can be checked and reasonably complied with (which I imagine would require some heuristic choices in the rule that we could then apply in the check).

> Consider how this situation is similar to the guidelines regarding pointers. The guidelines assume that a project isn't using old standards of C++. Where one has a huge legacy code base it will be impractical at best to try to apply the C++ Core Guidelines for pointer handling on the old code.
> 
> The expectation is that new projects will use the new libraries and language features available to do things differently, in a way that makes it possible  and practical to follow the guidelines; while legacy code remains unchecked or incrementally improved.
>  For any new code base this guideline shouldn't be a problem and existing projects can just adapt the usage of this rule to fit their situation by applying it selectively in the code base and/or changing the options.

This is a very different situation, IMO. This is a check that is almost impossible to comply with when developing new code because it is insufficiently clear on what constitutes "related" parameters. Two adjacent types in a function parameter list is an incredibly common situation that arises naturally when writing code (especially for constructors, where your parameter order has to match the declaration order within the class) and designing around that from scratch is not always possible. Retroactively applying this rule to an existing code base would be nearly impossible for a project of any size.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69560