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

[Kim Viggedal via Phabricator via cfe-commits]

vingeldal added a comment.

In D69560#1889719 <https://reviews.llvm.org/D69560#1889719>, @aaron.ballman wrote:

> 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.


I agree with this, the way I see it there are false positives and there is nothing "mushy" about the definitions of those.
Though I would prefer that, even if the implementation suggested in the guideline would be to crude, any deviations from what the C++ Core Guidelines specifies must be explicitly chosen by the user and not an option default.

>> 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).

Doesn't clang-tidy exclude warnings from system headers by default though?
Also I'm convinced this check would already be of use in many code bases, as it is today, (but sure, as you said, supporting data for that would be good) and therefor it would be a clear improvement to clang-tidy.
It would be better to use this check until we can do even better, rather than to hold of the check completely until we can implement it perfectly for every code base.

Would it not be a reasonable approach to accept this implementation and improve `NOLINT` to work on entire files or lists of files rather than squeezing more heuristics (prone to false negatives) to the check?



================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AdjacentArgumentsOfSameTypeCheck.cpp:474
+// Default to 3 so the users don't get a warning for every possible thing.
+static const unsigned DefaultMinLength = 3;
+
----------------
whisperity wrote:
> vingeldal wrote:
> > Am I getting this right, is this the number of consecutive arguments of the same type which is required for this check to print a diagnostic? If so: why not set the default value to 2? -I think that's what a user who just read the C++ Core Guidelines would expect.
> Well, it is to reduce the number of nagging errors. Setting it to `2` would result in every "min" and "pow" and similar functions to match. It's a compromise.
I think this value very well may strike a good balance in that compromise but I still think it is a clear deviation from the guideline as specified.
IMO all deviations from the guideline should be explicitly requested by the user, not set by default, no matter how useful that deviation is.


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

https://reviews.llvm.org/D69560