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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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

> I'd rather not call them //false positive// because the definition of `false positive` is ugly and mushy with regards to this check. This check attempts to enforce an interface rule, whether you(r project) wants to adhere the rule is a concise decision. A type equivalence (or convertibility in case of the follow-up patch D75041 <https://reviews.llvm.org/D75041> that considers implicit conversions) should be a "true positive" in every case, as an indicator of potentially bad design.


The problem is that there *are* false positives, logically. The rule is about "unrelated" parameters, so the false positives come from not having any way to distinguish between related and unrelated arguments. My intuition is that the simple enforcement suggested by the rule is unlikely to be acceptable, but that's why I'm asking for data over large, real-world projects.

> However, there's the question whether or not the similar argument-ness is intentional, i.e. a (from one PoV flawed) but deliberate design decision by the project. We've tested a few projects. The ugliest I've found is //OpenCV// where every variable of a function argument is either an `_InputArray` or an `_OutputArray`...
> 
> Another question is, how fragmented you want your type system to be...
>  I hope my research into this topic will be at least something fruitful. It's all about compromises, one end if using `any` for all your variables, the other extreme is having a specific type for every single occurrence (stuff like `fabs_return_t`, `create_schema_name_t` and stuff).

Agreed -- if we can find a heuristic that makes this work, the check has a lot of value.

>> In D74463#1888270 <https://reviews.llvm.org/D74463#1888270>, @aaron.ballman wrote:
>> There is no obvious way to silence any of the diagnostics generated as false positives short of requiring a naming convention for users to follow, which is not a particularly clean solution.
> 
> There is, you decide not to enable the checker?

That is not a useful level of granularity given that this check is likely to be incredibly noisy as part of our other C++ Core Guideline enforcement. We typically have `NOLINT` comments that we can fall back on for silencing individual instances of a false positive, but that's not likely to be a workable solution by itself for this check in particular because of system and third-party headers which are outside of the user's control (and unlikely to be willing or able to modify).


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

https://reviews.llvm.org/D69560