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

[Whisperity via Phabricator via cfe-commits]

whisperity marked an inline comment as done.
whisperity added a comment.

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.

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

There's a similar patch D20689 <https://reviews.llvm.org/D20689> but that uses names (and thus requires that arguments and parameters to be "named" in some way).



================
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;
+
----------------
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.


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

https://reviews.llvm.org/D69560