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

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

I'd like to resurrect the discussion about this. Unfortunately, the concept of 
`experimental-` group (in D76545 ) has fallen 
silent, too. We will present the results of this analysis soon at IEEE SCAM 
(Source Code Analysis and Manipulation) 2020 . While 
a previous submission about this topic was rejected on the grounds of not being 
in line with the conference's usual, hyper-formalised nature, with one reviewer 
//especially// praising the paper for its nice touch with the empirical world 
and the fact that neither argument swaps (another checker worked on by a 
colleague) nor the interactions of this issue with the type system (this 
checker) was really investigated in the literature for C++ before, we've 
tailored both the paper and the implementation based on reviewer comments from 
the scientific world, and the comments here.
The reviews were mostly favourable, excl. comments about the lack of 
formalisation and the unfortunate lack of modelling for template 
instantiations. Such a cold cut, however, //only// introduces false negatives 
into the result set. Which is fine, as the users will never see those 
non-existent reports!

In D69560#1936574 , @aaron.ballman 
wrote:

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

I agree the CppCG is rather vague about this. From an angle, it seems 
intentional... "relatedness" is hard to pin down formally, it might vary from 
one "module" to the next even in the same project. In a subsequent patch to 
this, I've given a few good examples as to what can be reasonably culled by 
relatedness heuristics. They filter a good chunk of the results, letting go of 
most of the trivially "valid (but maybe nonsense) if swapped" functions (min, 
max, find, etc.)

In D69560#1936574 , @aaron.ballman 
wrote:

> Two adjacent types in a function parameter list is an incredibly common 
> situation that arises naturally when writing code [...] and designing around 
> that from scratch is not always possible.

I see the reasoning behind this. However, this feels more like a motivation 
//against// the rule itself, and not the checker. We can debate the 
justification for the rule's existence, but (and correct me if I'm wrong) so 
far I have not seen any tool (that's publicly available, and is for C(++), 
etc...) that attempts to check your code satisfying this particular rule.

In D69560#1936574 , @aaron.ballman 
wrote:

> (especially for constructors, where your parameter order has to match the 
> declaration order within the class)

CMIIW, but I believe this statement is not true. Aggregate initialisation does 
not care about your constructors, and yes, takes arguments in the order of 
fields. However, once you have a user-defined constructor, you should be able 
to define your parameters in any order you like. The only thing you //should// 
match is that the //member-init-exprs// of the constructor are evaluated in 
order of field declarations, not in the order you wrote them. But trespassing 
that rule already emits a compiler warning, and in reality, the trespass only 
causes an issue if there is a data dependency between your fields. You 
//should// ensure your //member-init-exprs// are in the right order (to guard 
that a later change introducing a data dependency won't blow you up), but this 
has zero effect on the order of parameters of the constructor.

In D69560#1936574 , @aaron.ballman 
wrote:

> Retroactively applying this rule to an existing code base would be nearly 
> impossible for a project of any size.

It is hard in the general case (although there are some approaches and 
foundations that we could build upon with refactoring, I've taken up the mantle 
of spending some time with this research), but there are cases where 
"modernisation" efforts, especially tool-aided ones are already possible. I 
have been recently suggested reading this paper: //H. K. Wright: Incremental 
Type Migration Using Type Algebra // 
While I agree that the case study in the paper is rather domain-specific if we 
consider the example there: once a tool has identified a variable/expression to 
not be a `double` but instead `abseil::Duration`, from a function call where 
such a refactored argument is passed, you can (or, hopefully, need to) replace 
the type of the parameter. And one such "adjacent parameters of the same type" 
has been refactored.

My position with the paper, and this implementation, is that it is more useful 
for new projects than existing ones. It might definitely not be useful //for 
LLVM// (a humongous project!), and it might not be possible to fix //every// 

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

[Whisperity via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.rst:136-139
+typedef   T  element_type;
+typedef const Tconst_element_type;
+typedef   T &reference_type;
+typedef const T &  const_reference_type;

Why are these typos still here, `typedef`??? instead of `typename`.


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

https://reviews.llvm.org/D69560



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Whisperity via Phabricator via cfe-commits]

whisperity updated this revision to Diff 259320.
whisperity added a comment.

- Better organisation of code, cleanup of debug prints, fix nomenclature of 
functions
- Explicitly mention the first and last param of the range for clarity
- Downlift the logic of joining and breaking ranges to main patch (this isn't 
terribly useful at this point, but both subsequent improvements to the check 
depend on this change)
  - Basically no longer assume that if param N can be swapped with param 1, 
then it can also be swapped with 2, 3, etc. without checking.
- Made the list of ignored parameter and type names configurable as a checker 
option
- Changed the default value of `MinimumLength` to `2`, as prescribed in the 
guideline's text.


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

https://reviews.llvm.org/D69560

Files:
  clang-tools-extra/clang-tidy/experimental/CMakeLists.txt
  
clang-tools-extra/clang-tidy/experimental/CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.cpp
  
clang-tools-extra/clang-tidy/experimental/CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.h
  clang-tools-extra/clang-tidy/experimental/ExperimentalTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-cvr-on.cpp
  
clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-ignores.cpp
  
clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-minlen3.cpp
  
clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.c

Index: clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.c
@@ -0,0 +1,34 @@
+// RUN: %check_clang_tidy %s \
+// RUN:   experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.CVRMixPossible, value: 1} \
+// RUN:   ]}' --
+
+struct T {};
+
+void memcpy(struct T *restrict dest, const struct T *restrict src) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: 2 adjacent parameters for 'memcpy' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:32: note: the first parameter in this range is 'dest'
+// CHECK-MESSAGES: :[[@LINE-3]]:63: note: the last parameter in this range is 'src'
+// CHECK-MESSAGES: :[[@LINE-4]]:38: note: at a call site, 'const struct T * restrict' might bind with same force as 'struct T *restrict'
+
+void merge(struct T *dst, const struct T *src1, const struct T *src2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 3 adjacent parameters for 'merge' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:22: note: the first parameter in this range is 'dst'
+// CHECK-MESSAGES: :[[@LINE-3]]:65: note: the last parameter in this range is 'src2'
+// CHECK-MESSAGES: :[[@LINE-4]]:27: note: at a call site, 'const struct T *' might bind with same force as 'struct T *'
+// CHECK-MESSAGES: :[[@LINE-5]]:49: note: at a call site, 'const struct T *' might bind with same force as 'struct T *'
+
+int equals(struct T a, struct T b) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 2 adjacent parameters for 'equals' of similar type ('struct T') are
+// CHECK-MESSAGES: :[[@LINE-2]]:21: note: the first parameter in this range is 'a'
+// CHECK-MESSAGES: :[[@LINE-3]]:33: note: the last parameter in this range is 'b'
+
+typedef struct {
+  int x;
+} S;
+
+int equals2(S l, S r) { return l.x == r.x; }
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: 2 adjacent parameters for 'equals2' of similar type ('S') are
+// CHECK-MESSAGES: :[[@LINE-2]]:15: note: the first parameter in this range is 'l'
+// CHECK-MESSAGES: :[[@LINE-3]]:20: note: the last parameter in this range is 'r'
Index: clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-minlen3.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-minlen3.cpp
@@ -0,0 +1,205 @@
+// RUN: %check_clang_tidy %s \
+// RUN:   experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.MinimumLength, value: 3} \
+// 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D69560#1936153 , @vingeldal wrote:

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kim Viggedal via Phabricator via cfe-commits]

vingeldal added a comment.

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

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69560



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Whisperity via Phabricator via cfe-commits]

whisperity updated this revision to Diff 251841.
whisperity retitled this revision from "[clang-tidy] Add 
'cppcoreguidelines-avoid-adjacent-arguments-of-same-type' check" to 
"[clang-tidy] Add 
'experimental-cppcoreguidelines-avoid-adjacent-arguments-of-the-same-type' 
check".
whisperity edited the summary of this revision.
whisperity set the repository for this revision to rG LLVM Github Monorepo.
whisperity added a comment.

- Renamed check to 
**`experimental-`**`cppcoreguidelines-avoid-adjacent-`**`parameters`**`-of-`**`the`**`-same-type`.
- `s/argument/parameter/g` in the code and output where appropriate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69560

Files:
  clang-tools-extra/clang-tidy/experimental/CMakeLists.txt
  
clang-tools-extra/clang-tidy/experimental/CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.cpp
  
clang-tools-extra/clang-tidy/experimental/CppcoreguidelinesAvoidAdjacentParametersOfTheSameTypeCheck.h
  clang-tools-extra/clang-tidy/experimental/ExperimentalTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-cvr-on.cpp
  
clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-default.cpp
  
clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-verbose.cpp
  
clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.c

Index: clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.c
@@ -0,0 +1,31 @@
+// RUN: %check_clang_tidy %s \
+// RUN:   experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.MinimumLength, value: 2}, \
+// RUN: {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.CVRMixPossible, value: 1} \
+// RUN:   ]}' --
+
+struct T {};
+
+void memcpy(struct T *restrict dest, const struct T *restrict src) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: 2 adjacent parameters for 'memcpy' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:63: note: last parameter in the adjacent range
+// CHECK-MESSAGES: :[[@LINE-3]]:38: note: at a call site, 'const struct T * restrict' might bind with same force as 'struct T *restrict'
+
+void merge(struct T *dst, const struct T *src1, const struct T *src2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 3 adjacent parameters for 'merge' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:65: note: last parameter in the adjacent range
+// CHECK-MESSAGES: :[[@LINE-3]]:27: note: at a call site, 'const struct T *' might bind with same force as 'struct T *'
+// CHECK-MESSAGES: :[[@LINE-4]]:49: note: at a call site, 'const struct T *' might bind with same force as 'struct T *'
+
+int equals(struct T a, struct T b) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 2 adjacent parameters for 'equals' of similar type ('struct T') are
+// CHECK-MESSAGES: :[[@LINE-2]]:33: note: last parameter in the adjacent range
+
+typedef struct {
+  int x;
+} S;
+
+int equals2(S l, S r) { return l.x == r.x; }
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: 2 adjacent parameters for 'equals2' of similar type ('S') are
+// CHECK-MESSAGES: :[[@LINE-2]]:20: note: last parameter in the adjacent range
Index: clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-verbose.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type-verbose.cpp
@@ -0,0 +1,432 @@
+// RUN: %check_clang_tidy %s \
+// RUN:   experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.MinimumLength, value: 2}, \
+// RUN: {key: experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type.CVRMixPossible, value: 1} \
+// RUN:   ]}' --
+
+void library(void *vp, void *vp2, void *vp3, int n, int m);
+// NO-WARN: The user has no chance to change only declared (usually library)
+// functions, so no diagnostic is made.