[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity marked 3 inline comments as done.
whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:57
+   "Const_Reverse_Iterator",
+   "const_reverse_iterator"
+   "Constreverseiterator",

RKSimon wrote:
> @whisperity There is a missing comma at the end of the 
> "const_reverse_iterator" line causing implicit concatenation with the 
> following "Constreverseiterator" line.
Thanks... I'll get around to hotfixing this ASAP, and hope it can be backported 
to //13.0//...

(I wish there was a checker for this? )


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 'bugprone-easily-swappable-parameters' check

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:57
+   "Const_Reverse_Iterator",
+   "const_reverse_iterator"
+   "Constreverseiterator",

@whisperity There is a missing comma at the end of the "const_reverse_iterator" 
line causing implicit concatenation with the following "Constreverseiterator" 
line.


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 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity marked 2 inline comments as done.
whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:113-120
+#define FB(Name, K) MIX_##Name = (1ull << (K##ull - 1ull))
+
+  FB(None, 1),  //< Mix between the two parameters is not possible.
+  FB(Trivial, 2),   //< The two mix trivially, and are the exact same type.
+  FB(Canonical, 3), //< The two mix because the types refer to the same
+// CanonicalType, but we do not elaborate as to how.
+

alexfh wrote:
> whisperity wrote:
> > alexfh wrote:
> > > I'd switch to scoped enums (`enum class`), remove the macro, and use 
> > > simpler code like `None = 0x1, Trivial = 0x2, Canonical = 0x4`, etc.
> > `enum class` is a good idea!
> > 
> > Unfortunately, I'm not really sold on the hexa literals... It makes the 
> > code confusing the read, because you have to keep in mind which range the 
> > hexa literals mean, and when there is a shift in position, e.g. if you read 
> > `0x20` you have to count out that okay, that's 2nd position, so at least 
> > 16, so at least the 4th bit, but it is two, so you shift it again to the 
> > right, so in the bit pattern it turns on the 5th bit actually.
> > It makes it very easy to misread or misinterpret things, especially when 
> > trying to work on the source code.
> > (In the last extension patch, the number of elements in the bit mask go up 
> > to the 8th or the 9th bit, I don't remember exactly at the top of my head.)
> > 
> > Compared to this, the current text when read immediately tells you which 
> > "decimal" position you should expect the appropriate bit to be toggled on, 
> > if the flag is there.
> > 
> > Binary literals, on the other hand, would be amazing to use here, sadly 
> > we're only in C++14.
> First of all, do we ever need to know the position of the set bit in these 
> flag enumerators? Isn't it enough that each enumerator has a distinct bit 
> set? If so, there's not much complexity in reading and expanding sequences 
> like `0x0001, 0x0002, 0x0004, 0x0008, 0x0010, 0x0020, 0x0040, 0x0080, 0x0100, 
> 0x0200, ...`.
> However, if that's not to your taste, there's another frequently used style 
> of defining flag enumerations in LLVM: `1 << 0, 1 << 1, 1 << 2, 1 << 3, ...`.
> 
> Either of these is fine, but using macros for this purpose is not necessary 
> and doesn't make the code any easier to read or to modify.
> 
> > Compared to this, the current text when read immediately tells you which 
> > "decimal" position you should expect the appropriate bit to be toggled on, 
> > if the flag is there.
> Do I understand correctly that you need this exclusively for the purpose of 
> reading debug logs? If so, it seems to be better to decode the bits before 
> logging them. See the comment on line 87 above.
Yes, there individual positions themselves don't matter! The only thing that 
matters is that some flags are < than `None` and the rest are `>=`, but that's 
only needed because of the ugly workaround.

I turned the debug printouts to use a manual formatting with "reasonable" 
characters being set (e.g. `T` for `Trivial` or `&` for `reference`). It's much 
more readable and straight-forward, because you actually do not need to know 
the position of the bits themselves, those don't matter.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:133
+
+  void sanitize() {
+assert(Flags != MIX_Invalid && "sanitize() called on invalid bitvec");

alexfh wrote:
> whisperity wrote:
> > alexfh wrote:
> > > What should this method do and how it should be used? Same for 
> > > Mix::sanitize()
> > Once there are more flags in the bitmask (not just "None" and "Trivial"), 
> > this method deals with sanitising the flags that are toggled on and off by 
> > the recursive descent on the type tree.
> > 
> > For example, if there is a typedef involved (see D95736), it will toggle on 
> > the typedef flag, and start investigating the aliased type. This subsequent 
> > investigation might figure out that the aliased type is trivial, or it is 
> > distinct. In case it is distinct, that level of the recursion will turn on 
> > the "Not mixable" flag. In this case, when the investigation returns 
> > completely, we will have "Typedef" and "None" both turned on. This method 
> > will throw away everything but the "None" bit, so when the data is returned 
> > to the diagnostic-generating part of the check, it will know what to do.
> Thanks for the explanation! But it would be very helpful to have this 
> information in the code, not only in a code review comment.
The full patch stack landed, and this function has a much more elaborated 
implementation (with more comments), I believe. In this patch, there was indeed 
nothing to add here.



Comment at: 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

I thought I sent this batch of comments loong ago =\

Better late than never.




Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:113-120
+#define FB(Name, K) MIX_##Name = (1ull << (K##ull - 1ull))
+
+  FB(None, 1),  //< Mix between the two parameters is not possible.
+  FB(Trivial, 2),   //< The two mix trivially, and are the exact same type.
+  FB(Canonical, 3), //< The two mix because the types refer to the same
+// CanonicalType, but we do not elaborate as to how.
+

whisperity wrote:
> alexfh wrote:
> > I'd switch to scoped enums (`enum class`), remove the macro, and use 
> > simpler code like `None = 0x1, Trivial = 0x2, Canonical = 0x4`, etc.
> `enum class` is a good idea!
> 
> Unfortunately, I'm not really sold on the hexa literals... It makes the code 
> confusing the read, because you have to keep in mind which range the hexa 
> literals mean, and when there is a shift in position, e.g. if you read `0x20` 
> you have to count out that okay, that's 2nd position, so at least 16, so at 
> least the 4th bit, but it is two, so you shift it again to the right, so in 
> the bit pattern it turns on the 5th bit actually.
> It makes it very easy to misread or misinterpret things, especially when 
> trying to work on the source code.
> (In the last extension patch, the number of elements in the bit mask go up to 
> the 8th or the 9th bit, I don't remember exactly at the top of my head.)
> 
> Compared to this, the current text when read immediately tells you which 
> "decimal" position you should expect the appropriate bit to be toggled on, if 
> the flag is there.
> 
> Binary literals, on the other hand, would be amazing to use here, sadly we're 
> only in C++14.
First of all, do we ever need to know the position of the set bit in these flag 
enumerators? Isn't it enough that each enumerator has a distinct bit set? If 
so, there's not much complexity in reading and expanding sequences like 
`0x0001, 0x0002, 0x0004, 0x0008, 0x0010, 0x0020, 0x0040, 0x0080, 0x0100, 
0x0200, ...`.
However, if that's not to your taste, there's another frequently used style of 
defining flag enumerations in LLVM: `1 << 0, 1 << 1, 1 << 2, 1 << 3, ...`.

Either of these is fine, but using macros for this purpose is not necessary and 
doesn't make the code any easier to read or to modify.

> Compared to this, the current text when read immediately tells you which 
> "decimal" position you should expect the appropriate bit to be toggled on, if 
> the flag is there.
Do I understand correctly that you need this exclusively for the purpose of 
reading debug logs? If so, it seems to be better to decode the bits before 
logging them. See the comment on line 87 above.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:133
+
+  void sanitize() {
+assert(Flags != MIX_Invalid && "sanitize() called on invalid bitvec");

whisperity wrote:
> alexfh wrote:
> > What should this method do and how it should be used? Same for 
> > Mix::sanitize()
> Once there are more flags in the bitmask (not just "None" and "Trivial"), 
> this method deals with sanitising the flags that are toggled on and off by 
> the recursive descent on the type tree.
> 
> For example, if there is a typedef involved (see D95736), it will toggle on 
> the typedef flag, and start investigating the aliased type. This subsequent 
> investigation might figure out that the aliased type is trivial, or it is 
> distinct. In case it is distinct, that level of the recursion will turn on 
> the "Not mixable" flag. In this case, when the investigation returns 
> completely, we will have "Typedef" and "None" both turned on. This method 
> will throw away everything but the "None" bit, so when the data is returned 
> to the diagnostic-generating part of the check, it will know what to do.
Thanks for the explanation! But it would be very helpful to have this 
information in the code, not only in a code review comment.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:129
+
+void qualified1(int I, const int CI) {} // NO-WARN: Not the same type.
+

whisperity wrote:
> alexfh wrote:
> > Does the top-level const change anything with regard to the "mixability"? 
> > It's a purely implementation-side difference, callers could still swap 
> > parameters.
> Unfortunately, coding styles differ whether people mark local variables const 
> or not, across projects. Even with pointers: in some cases, a swapped call to 
> `memcpy` might be caught if the user made one of the pointers passed already 
> `const void*` instead of just `void*`.
> 
> D95736 deals with implementing the logic for this, it is exposed as a 
> user-configurable option whether they want to consider const stuff the same 
> as non-const stuff.
The `const` in 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG499e39c5983d: [clang-tidy] Add 
bugprone-easily-swappable-parameters check (authored by whisperity).

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/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
@@ -0,0 +1,148 @@
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: "bool;MyBool;struct U;MAKE_LOGICAL_TYPE(int)"} \
+// RUN:  ]}' -- -x c
+
+#define bool _Bool
+#define true 1
+#define false 0
+
+typedef bool MyBool;
+
+#define TheLogicalType bool
+
+void declVoid(void); // NO-WARN: Declaration only.
+void decl(); // NO-WARN: Declaration only.
+void oneParam(int I) {}  // NO-WARN: 1 parameter.
+void variadic(int I, ...) {} // NO-WARN: 1 visible parameter.
+
+void trivial(int I, int J) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 2 adjacent parameters of 'trivial' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:18: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:25: note: the last parameter in the range is 'J'
+
+void qualifier(int I, const int CI) {} // NO-WARN: Distinct types.
+
+void restrictQualifier(char *restrict CPR1, char *restrict CPR2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 2 adjacent parameters of 'restrictQualifier' of similar type ('char *restrict')
+// CHECK-MESSAGES: :[[@LINE-2]]:39: note: the first parameter in the range is 'CPR1'
+// CHECK-MESSAGES: :[[@LINE-3]]:60: note: the last parameter in the range is 'CPR2'
+
+void pointer1(int *IP1, int *IP2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 2 adjacent parameters of 'pointer1' of similar type ('int *')
+// CHECK-MESSAGES: :[[@LINE-2]]:20: note: the first parameter in the range is 'IP1'
+// CHECK-MESSAGES: :[[@LINE-3]]:30: note: the last parameter in the range is 'IP2'
+
+void pointerConversion(int *IP, long *LP) {}
+// NO-WARN: Even though C can convert any T* to U* back and forth, compiler
+// warnings already exist for this.
+
+void testVariadicsCall() {
+  int IVal = 1;
+  decl(IVal); // NO-WARN: Particular calls to "variadics" are like template
+  // instantiations, and we do not model them.
+
+  variadic(IVal);  // NO-WARN.
+  variadic(IVal, 2, 3, 4); // NO-WARN.
+}
+
+struct S {};
+struct T {};
+
+void taggedTypes1(struct S SVar, struct T TVar) {} // NO-WARN: Distinct types.
+
+void taggedTypes2(struct S SVar1, struct S SVar2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 2 adjacent parameters of 'taggedTypes2' of similar type ('struct S')
+// CHECK-MESSAGES: :[[@LINE-2]]:28: note: the first parameter in the range is 'SVar1'
+// CHECK-MESSAGES: :[[@LINE-3]]:44: note: the last parameter in the range is 'SVar2'
+
+void wrappers(struct { int I; } I1, struct { int I; } I2) {} // NO-WARN: Distinct anonymous types.
+
+void knr(I, J)
+  int I;
+  int J;
+{}
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: 2 adjacent parameters of 'knr' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-4]]:7: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-4]]:7: note: the last parameter in the range is 'J'
+
+void boolAsWritten(bool B1, bool B2) {} // NO-WARN: The type name is ignored.
+// Note that "bool" is a macro that expands to "_Bool" internally, but it is
+// only "bool" that is ignored from the two.
+
+void underscoreBoolAsWritten(_Bool B1, _Bool B2) {}
+// Even though it is "_Bool" that is written in the code, the diagnostic message
+// 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D69560#2840579 , @whisperity wrote:

> In D69560#2840554 , @aaron.ballman 
> wrote:
>
>> This is a very involved check that I think is going to produce some 
>> interesting results for users, thank you for working so diligently on it! I 
>> think all of the patches in the series have now been accepted and this is 
>> ready to land. @whisperity, are you aware of any changes that are left to be 
>> made?
>
> I'll ping @martong privately if they have additional comments about D75041 
> , but otherwise, no, I think no more things 
> left to do. Landing checklist complete.

Great! Unless you hear from @alexfh, @martong, or someone else with additional 
concerns, I think you're set to land this!


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 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D69560#2840554 , @aaron.ballman 
wrote:

> This is a very involved check that I think is going to produce some 
> interesting results for users, thank you for working so diligently on it! I 
> think all of the patches in the series have now been accepted and this is 
> ready to land. @whisperity, are you aware of any changes that are left to be 
> made?

I'll ping @martong privately if they have additional comments about D75041 
, but otherwise, no, I think no more things 
left to do. Landing checklist complete.


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 'bugprone-easily-swappable-parameters' check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

This is a very involved check that I think is going to produce some interesting 
results for users, thank you for working so diligently on it! I think all of 
the patches in the series have now been accepted and this is ready to land. 
@whisperity, are you aware of any changes that are left to be made?


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 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

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

Attempt to fix build failure on Windows buildbot (`operator new` was taking 
`unsigned long` instead of `size_t` in a test which resulted in hard error).


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/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
@@ -0,0 +1,148 @@
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: "bool;MyBool;struct U;MAKE_LOGICAL_TYPE(int)"} \
+// RUN:  ]}' -- -x c
+
+#define bool _Bool
+#define true 1
+#define false 0
+
+typedef bool MyBool;
+
+#define TheLogicalType bool
+
+void declVoid(void); // NO-WARN: Declaration only.
+void decl(); // NO-WARN: Declaration only.
+void oneParam(int I) {}  // NO-WARN: 1 parameter.
+void variadic(int I, ...) {} // NO-WARN: 1 visible parameter.
+
+void trivial(int I, int J) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 2 adjacent parameters of 'trivial' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:18: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:25: note: the last parameter in the range is 'J'
+
+void qualifier(int I, const int CI) {} // NO-WARN: Distinct types.
+
+void restrictQualifier(char *restrict CPR1, char *restrict CPR2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 2 adjacent parameters of 'restrictQualifier' of similar type ('char *restrict')
+// CHECK-MESSAGES: :[[@LINE-2]]:39: note: the first parameter in the range is 'CPR1'
+// CHECK-MESSAGES: :[[@LINE-3]]:60: note: the last parameter in the range is 'CPR2'
+
+void pointer1(int *IP1, int *IP2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 2 adjacent parameters of 'pointer1' of similar type ('int *')
+// CHECK-MESSAGES: :[[@LINE-2]]:20: note: the first parameter in the range is 'IP1'
+// CHECK-MESSAGES: :[[@LINE-3]]:30: note: the last parameter in the range is 'IP2'
+
+void pointerConversion(int *IP, long *LP) {}
+// NO-WARN: Even though C can convert any T* to U* back and forth, compiler
+// warnings already exist for this.
+
+void testVariadicsCall() {
+  int IVal = 1;
+  decl(IVal); // NO-WARN: Particular calls to "variadics" are like template
+  // instantiations, and we do not model them.
+
+  variadic(IVal);  // NO-WARN.
+  variadic(IVal, 2, 3, 4); // NO-WARN.
+}
+
+struct S {};
+struct T {};
+
+void taggedTypes1(struct S SVar, struct T TVar) {} // NO-WARN: Distinct types.
+
+void taggedTypes2(struct S SVar1, struct S SVar2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 2 adjacent parameters of 'taggedTypes2' of similar type ('struct S')
+// CHECK-MESSAGES: :[[@LINE-2]]:28: note: the first parameter in the range is 'SVar1'
+// CHECK-MESSAGES: :[[@LINE-3]]:44: note: the last parameter in the range is 'SVar2'
+
+void wrappers(struct { int I; } I1, struct { int I; } I2) {} // NO-WARN: Distinct anonymous types.
+
+void knr(I, J)
+  int I;
+  int J;
+{}
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: 2 adjacent parameters of 'knr' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-4]]:7: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-4]]:7: note: the last parameter in the range is 'J'
+
+void boolAsWritten(bool B1, bool B2) {} // NO-WARN: The type name is ignored.
+// Note that "bool" is a macro that expands to "_Bool" internally, but it is
+// only "bool" that is ignored from the two.
+
+void underscoreBoolAsWritten(_Bool B1, _Bool B2) {}
+// Even though it is "_Bool" that is written in the 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

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

(Try nudging the CI to only apply this commit after removal of parent links.)


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/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
@@ -0,0 +1,148 @@
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: "bool;MyBool;struct U;MAKE_LOGICAL_TYPE(int)"} \
+// RUN:  ]}' -- -x c
+
+#define bool _Bool
+#define true 1
+#define false 0
+
+typedef bool MyBool;
+
+#define TheLogicalType bool
+
+void declVoid(void); // NO-WARN: Declaration only.
+void decl(); // NO-WARN: Declaration only.
+void oneParam(int I) {}  // NO-WARN: 1 parameter.
+void variadic(int I, ...) {} // NO-WARN: 1 visible parameter.
+
+void trivial(int I, int J) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 2 adjacent parameters of 'trivial' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:18: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:25: note: the last parameter in the range is 'J'
+
+void qualifier(int I, const int CI) {} // NO-WARN: Distinct types.
+
+void restrictQualifier(char *restrict CPR1, char *restrict CPR2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 2 adjacent parameters of 'restrictQualifier' of similar type ('char *restrict')
+// CHECK-MESSAGES: :[[@LINE-2]]:39: note: the first parameter in the range is 'CPR1'
+// CHECK-MESSAGES: :[[@LINE-3]]:60: note: the last parameter in the range is 'CPR2'
+
+void pointer1(int *IP1, int *IP2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 2 adjacent parameters of 'pointer1' of similar type ('int *')
+// CHECK-MESSAGES: :[[@LINE-2]]:20: note: the first parameter in the range is 'IP1'
+// CHECK-MESSAGES: :[[@LINE-3]]:30: note: the last parameter in the range is 'IP2'
+
+void pointerConversion(int *IP, long *LP) {}
+// NO-WARN: Even though C can convert any T* to U* back and forth, compiler
+// warnings already exist for this.
+
+void testVariadicsCall() {
+  int IVal = 1;
+  decl(IVal); // NO-WARN: Particular calls to "variadics" are like template
+  // instantiations, and we do not model them.
+
+  variadic(IVal);  // NO-WARN.
+  variadic(IVal, 2, 3, 4); // NO-WARN.
+}
+
+struct S {};
+struct T {};
+
+void taggedTypes1(struct S SVar, struct T TVar) {} // NO-WARN: Distinct types.
+
+void taggedTypes2(struct S SVar1, struct S SVar2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 2 adjacent parameters of 'taggedTypes2' of similar type ('struct S')
+// CHECK-MESSAGES: :[[@LINE-2]]:28: note: the first parameter in the range is 'SVar1'
+// CHECK-MESSAGES: :[[@LINE-3]]:44: note: the last parameter in the range is 'SVar2'
+
+void wrappers(struct { int I; } I1, struct { int I; } I2) {} // NO-WARN: Distinct anonymous types.
+
+void knr(I, J)
+  int I;
+  int J;
+{}
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: 2 adjacent parameters of 'knr' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-4]]:7: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-4]]:7: note: the last parameter in the range is 'J'
+
+void boolAsWritten(bool B1, bool B2) {} // NO-WARN: The type name is ignored.
+// Note that "bool" is a macro that expands to "_Bool" internally, but it is
+// only "bool" that is ignored from the two.
+
+void underscoreBoolAsWritten(_Bool B1, _Bool B2) {}
+// Even though it is "_Bool" that is written in the code, the diagnostic message
+// respects the printing policy as defined by 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D69560#2747092 , @alexfh wrote:

> BTW, any ideas why "patch application failed"? (See the Build status in the 
> Diff Detail section)

I think I figured out why the pre-build CI isn't working... it seems that the 
fact that this patch depends on an abandoned revision D76545 
 misleads the CI logic as it sees the 
dependency, and even though it is abandoned, tries to download and apply the 
contents of D76545 , which makes it fail...

  INFOD76545#251840 commit 1e9b6b89a7b5c49612018b120c2c142106056f82 has a 
later commit date then7cdbf1ed4b94259a3aad2a7575e928fa61b0e57e
  [...]
  ERROR   error: patch failed: clang-tools-extra/docs/clang-tidy/index.rst:62
  error: clang-tools-extra/docs/clang-tidy/index.rst: patch does not apply
  error: patch failed: clang-tools-extra/clang-tidy/ClangTidyForceLinker.h:15
  error: clang-tools-extra/clang-tidy/ClangTidyForceLinker.h: patch does not 
apply


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 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity updated this revision to Diff 353969.
whisperity marked 8 inline comments as done.
whisperity added a comment.

All changes are **NFC** and styling only.

- Remove the `FB` macro in favour of explicitly specifying the bit flag value
- Change debug printout from bit pattern to individual significant letters


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/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
@@ -0,0 +1,148 @@
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: "bool;MyBool;struct U;MAKE_LOGICAL_TYPE(int)"} \
+// RUN:  ]}' -- -x c
+
+#define bool _Bool
+#define true 1
+#define false 0
+
+typedef bool MyBool;
+
+#define TheLogicalType bool
+
+void declVoid(void); // NO-WARN: Declaration only.
+void decl(); // NO-WARN: Declaration only.
+void oneParam(int I) {}  // NO-WARN: 1 parameter.
+void variadic(int I, ...) {} // NO-WARN: 1 visible parameter.
+
+void trivial(int I, int J) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 2 adjacent parameters of 'trivial' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:18: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:25: note: the last parameter in the range is 'J'
+
+void qualifier(int I, const int CI) {} // NO-WARN: Distinct types.
+
+void restrictQualifier(char *restrict CPR1, char *restrict CPR2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 2 adjacent parameters of 'restrictQualifier' of similar type ('char *restrict')
+// CHECK-MESSAGES: :[[@LINE-2]]:39: note: the first parameter in the range is 'CPR1'
+// CHECK-MESSAGES: :[[@LINE-3]]:60: note: the last parameter in the range is 'CPR2'
+
+void pointer1(int *IP1, int *IP2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 2 adjacent parameters of 'pointer1' of similar type ('int *')
+// CHECK-MESSAGES: :[[@LINE-2]]:20: note: the first parameter in the range is 'IP1'
+// CHECK-MESSAGES: :[[@LINE-3]]:30: note: the last parameter in the range is 'IP2'
+
+void pointerConversion(int *IP, long *LP) {}
+// NO-WARN: Even though C can convert any T* to U* back and forth, compiler
+// warnings already exist for this.
+
+void testVariadicsCall() {
+  int IVal = 1;
+  decl(IVal); // NO-WARN: Particular calls to "variadics" are like template
+  // instantiations, and we do not model them.
+
+  variadic(IVal);  // NO-WARN.
+  variadic(IVal, 2, 3, 4); // NO-WARN.
+}
+
+struct S {};
+struct T {};
+
+void taggedTypes1(struct S SVar, struct T TVar) {} // NO-WARN: Distinct types.
+
+void taggedTypes2(struct S SVar1, struct S SVar2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 2 adjacent parameters of 'taggedTypes2' of similar type ('struct S')
+// CHECK-MESSAGES: :[[@LINE-2]]:28: note: the first parameter in the range is 'SVar1'
+// CHECK-MESSAGES: :[[@LINE-3]]:44: note: the last parameter in the range is 'SVar2'
+
+void wrappers(struct { int I; } I1, struct { int I; } I2) {} // NO-WARN: Distinct anonymous types.
+
+void knr(I, J)
+  int I;
+  int J;
+{}
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: 2 adjacent parameters of 'knr' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-4]]:7: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-4]]:7: note: the last parameter in the range is 'J'
+
+void boolAsWritten(bool B1, bool B2) {} // NO-WARN: The type name is ignored.
+// Note that "bool" is a macro that expands to "_Bool" internally, but it is
+// only "bool" that is ignored from the two.
+
+void 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

There was a bit of talk on Twitter recently about strong typing which reminded 
me of this checker!  I'll fix all issues in one go once we're through with all 
the comments, I'd like to see the full picture of what needs to be done.


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 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D69560#2747092 , @alexfh wrote:

> BTW, any ideas why "patch application failed"? (See the Build status in the 
> Diff Detail section)
>
> It would be really nice to get pre-merge checks to work for the patch.

Put simply, I think I haven't rebased the patch since a few changes ago. The 
past few changes were NFC/lint stuff only.




Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:113-120
+#define FB(Name, K) MIX_##Name = (1ull << (K##ull - 1ull))
+
+  FB(None, 1),  //< Mix between the two parameters is not possible.
+  FB(Trivial, 2),   //< The two mix trivially, and are the exact same type.
+  FB(Canonical, 3), //< The two mix because the types refer to the same
+// CanonicalType, but we do not elaborate as to how.
+

alexfh wrote:
> I'd switch to scoped enums (`enum class`), remove the macro, and use simpler 
> code like `None = 0x1, Trivial = 0x2, Canonical = 0x4`, etc.
`enum class` is a good idea!

Unfortunately, I'm not really sold on the hexa literals... It makes the code 
confusing the read, because you have to keep in mind which range the hexa 
literals mean, and when there is a shift in position, e.g. if you read `0x20` 
you have to count out that okay, that's 2nd position, so at least 16, so at 
least the 4th bit, but it is two, so you shift it again to the right, so in the 
bit pattern it turns on the 5th bit actually.
It makes it very easy to misread or misinterpret things, especially when trying 
to work on the source code.
(In the last extension patch, the number of elements in the bit mask go up to 
the 8th or the 9th bit, I don't remember exactly at the top of my head.)

Compared to this, the current text when read immediately tells you which 
"decimal" position you should expect the appropriate bit to be toggled on, if 
the flag is there.

Binary literals, on the other hand, would be amazing to use here, sadly we're 
only in C++14.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:133
+
+  void sanitize() {
+assert(Flags != MIX_Invalid && "sanitize() called on invalid bitvec");

alexfh wrote:
> What should this method do and how it should be used? Same for Mix::sanitize()
Once there are more flags in the bitmask (not just "None" and "Trivial"), this 
method deals with sanitising the flags that are toggled on and off by the 
recursive descent on the type tree.

For example, if there is a typedef involved (see D95736), it will toggle on the 
typedef flag, and start investigating the aliased type. This subsequent 
investigation might figure out that the aliased type is trivial, or it is 
distinct. In case it is distinct, that level of the recursion will turn on the 
"Not mixable" flag. In this case, when the investigation returns completely, we 
will have "Typedef" and "None" both turned on. This method will throw away 
everything but the "None" bit, so when the data is returned to the 
diagnostic-generating part of the check, it will know what to do.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:313-320
+if (B.isMacroID()) {
+  LLVM_DEBUG(llvm::dbgs() << "\t\tBeginning is macro.\n");
+  B = SM.getTopMacroCallerLoc(B);
+}
+if (E.isMacroID()) {
+  LLVM_DEBUG(llvm::dbgs() << "\t\tEnding is macro.\n");
+  E = Lexer::getLocForEndOfToken(SM.getTopMacroCallerLoc(E), 0, SM, LO);

alexfh wrote:
> Is there a chance that you're trying to reimplement a part of 
> Lexer::makeFileCharRange functionality here?
Does that expand the token length properly? I took this idiom of 
`getLocForEndOfToken` from various other Tidy checks... basically, anything 
that deals with replacing some sort of textual range in the code calls to this 
function. I need to have the exact ending location so I can obtain the full 
text, as appears in the source file. In this case, to compare against the 
user's input.

E.g. here is one example from `utils/UsingInserter.cpp`:

```
  32   │ Optional UsingInserter::createUsingDeclaration(
  33   │ ASTContext , const Stmt , StringRef 
QualifiedName) {
/* ... */
  42   │   SourceLocation InsertLoc = Lexer::getLocForEndOfToken(
  43   │   Function->getBody()->getBeginLoc(), 0, SourceMgr, 
Context.getLangOpts());
```

A quick grep for `makeCharRange` in the repository didn't turn up any 
significant result, apart from one unit test.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:129
+
+void qualified1(int I, const int CI) {} // NO-WARN: Not the same type.
+

alexfh wrote:
> Does the top-level const change anything with regard to the "mixability"? 
> It's a purely implementation-side difference, callers could still swap 
> parameters.
Unfortunately, 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

BTW, any ideas why "patch application failed"? (See the Build status in the 
Diff Detail section)

It would be really nice to get pre-merge checks to work for the patch.


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 'bugprone-easily-swappable-parameters' check

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Thanks for the great work! This is mostly looking fine, but I've added a few 
comments. I could only make a quite superficial review so far, but I'll try to 
take a deeper look into this soon.




Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:70
+  static constexpr std::size_t WidthWithPadding = Width + (Width / 4);
+  char S[WidthWithPadding];
+  for (std::size_t CurPos = 0; CurPos < WidthWithPadding; ++CurPos) {

I think, this function is not needed (see the comment below), but if it is, a 
single std::string and in-place std::reverse() would be better, imo.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:84-87
+/// Formats the bit representation of a numeric type as a string in groups of 
4.
+template  static inline std::string formatBits(T &) {
+  return formatBitsImpl(V);
+}

I suppose that textual representation of the flags would be easier to 
understand in debug logs than just bits. Maybe single characters or other 
abbreviations, if this needs to be compact.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:113-120
+#define FB(Name, K) MIX_##Name = (1ull << (K##ull - 1ull))
+
+  FB(None, 1),  //< Mix between the two parameters is not possible.
+  FB(Trivial, 2),   //< The two mix trivially, and are the exact same type.
+  FB(Canonical, 3), //< The two mix because the types refer to the same
+// CanonicalType, but we do not elaborate as to how.
+

I'd switch to scoped enums (`enum class`), remove the macro, and use simpler 
code like `None = 0x1, Trivial = 0x2, Canonical = 0x4`, etc.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:133
+
+  void sanitize() {
+assert(Flags != MIX_Invalid && "sanitize() called on invalid bitvec");

What should this method do and how it should be used? Same for Mix::sanitize()



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:295-297
+  if (llvm::any_of(
+  Check.IgnoredParameterNames,
+  [NodeName](const std::string ) { return NodeName == E; })) {

Would `llvm::find(Check.IgnoredParameterNames, NodeName) != 
Check.IgnoredParameterNames.end()` work?



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:302
+
+  StringRef NodeTypeName = [Node]() {
+const ASTContext  = Node->getASTContext();

No need for parentheses here.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:313-320
+if (B.isMacroID()) {
+  LLVM_DEBUG(llvm::dbgs() << "\t\tBeginning is macro.\n");
+  B = SM.getTopMacroCallerLoc(B);
+}
+if (E.isMacroID()) {
+  LLVM_DEBUG(llvm::dbgs() << "\t\tEnding is macro.\n");
+  E = Lexer::getLocForEndOfToken(SM.getTopMacroCallerLoc(E), 0, SM, LO);

Is there a chance that you're trying to reimplement a part of 
Lexer::makeFileCharRange functionality here?



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:129
+
+void qualified1(int I, const int CI) {} // NO-WARN: Not the same type.
+

Does the top-level const change anything with regard to the "mixability"? It's 
a purely implementation-side difference, callers could still swap parameters.


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 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

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

Made sure that the parameter's name is highlighted promptly in the 
//"first|last parameter of the range is..."// note.


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/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
@@ -0,0 +1,148 @@
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: "bool;MyBool;struct U;MAKE_LOGICAL_TYPE(int)"} \
+// RUN:  ]}' -- -x c
+
+#define bool _Bool
+#define true 1
+#define false 0
+
+typedef bool MyBool;
+
+#define TheLogicalType bool
+
+void declVoid(void); // NO-WARN: Declaration only.
+void decl(); // NO-WARN: Declaration only.
+void oneParam(int I) {}  // NO-WARN: 1 parameter.
+void variadic(int I, ...) {} // NO-WARN: 1 visible parameter.
+
+void trivial(int I, int J) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 2 adjacent parameters of 'trivial' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:18: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:25: note: the last parameter in the range is 'J'
+
+void qualifier(int I, const int CI) {} // NO-WARN: Distinct types.
+
+void restrictQualifier(char *restrict CPR1, char *restrict CPR2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 2 adjacent parameters of 'restrictQualifier' of similar type ('char *restrict')
+// CHECK-MESSAGES: :[[@LINE-2]]:39: note: the first parameter in the range is 'CPR1'
+// CHECK-MESSAGES: :[[@LINE-3]]:60: note: the last parameter in the range is 'CPR2'
+
+void pointer1(int *IP1, int *IP2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 2 adjacent parameters of 'pointer1' of similar type ('int *')
+// CHECK-MESSAGES: :[[@LINE-2]]:20: note: the first parameter in the range is 'IP1'
+// CHECK-MESSAGES: :[[@LINE-3]]:30: note: the last parameter in the range is 'IP2'
+
+void pointerConversion(int *IP, long *LP) {}
+// NO-WARN: Even though C can convert any T* to U* back and forth, compiler
+// warnings already exist for this.
+
+void testVariadicsCall() {
+  int IVal = 1;
+  decl(IVal); // NO-WARN: Particular calls to "variadics" are like template
+  // instantiations, and we do not model them.
+
+  variadic(IVal);  // NO-WARN.
+  variadic(IVal, 2, 3, 4); // NO-WARN.
+}
+
+struct S {};
+struct T {};
+
+void taggedTypes1(struct S SVar, struct T TVar) {} // NO-WARN: Distinct types.
+
+void taggedTypes2(struct S SVar1, struct S SVar2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 2 adjacent parameters of 'taggedTypes2' of similar type ('struct S')
+// CHECK-MESSAGES: :[[@LINE-2]]:28: note: the first parameter in the range is 'SVar1'
+// CHECK-MESSAGES: :[[@LINE-3]]:44: note: the last parameter in the range is 'SVar2'
+
+void wrappers(struct { int I; } I1, struct { int I; } I2) {} // NO-WARN: Distinct anonymous types.
+
+void knr(I, J)
+  int I;
+  int J;
+{}
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: 2 adjacent parameters of 'knr' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-4]]:7: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-4]]:7: note: the last parameter in the range is 'J'
+
+void boolAsWritten(bool B1, bool B2) {} // NO-WARN: The type name is ignored.
+// Note that "bool" is a macro that expands to "_Bool" internally, but it is
+// only "bool" that is ignored from the two.
+
+void underscoreBoolAsWritten(_Bool B1, _Bool B2) {}
+// Even though it is "_Bool" that is written in the code, the diagnostic message
+// 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity updated this revision to Diff 336618.
whisperity set the repository for this revision to rG LLVM Github Monorepo.
whisperity added a comment.

Rebased over D98635 , allowing us to properly 
highlight the found //"mixable adjacent parameter range"// (assuming it's on 
the same line, of course):

F16236992: UnderSquiggly.png 


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/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
@@ -0,0 +1,148 @@
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: "bool;MyBool;struct U;MAKE_LOGICAL_TYPE(int)"} \
+// RUN:  ]}' -- -x c
+
+#define bool _Bool
+#define true 1
+#define false 0
+
+typedef bool MyBool;
+
+#define TheLogicalType bool
+
+void declVoid(void); // NO-WARN: Declaration only.
+void decl(); // NO-WARN: Declaration only.
+void oneParam(int I) {}  // NO-WARN: 1 parameter.
+void variadic(int I, ...) {} // NO-WARN: 1 visible parameter.
+
+void trivial(int I, int J) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 2 adjacent parameters of 'trivial' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:18: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:25: note: the last parameter in the range is 'J'
+
+void qualifier(int I, const int CI) {} // NO-WARN: Distinct types.
+
+void restrictQualifier(char *restrict CPR1, char *restrict CPR2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 2 adjacent parameters of 'restrictQualifier' of similar type ('char *restrict')
+// CHECK-MESSAGES: :[[@LINE-2]]:39: note: the first parameter in the range is 'CPR1'
+// CHECK-MESSAGES: :[[@LINE-3]]:60: note: the last parameter in the range is 'CPR2'
+
+void pointer1(int *IP1, int *IP2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 2 adjacent parameters of 'pointer1' of similar type ('int *')
+// CHECK-MESSAGES: :[[@LINE-2]]:20: note: the first parameter in the range is 'IP1'
+// CHECK-MESSAGES: :[[@LINE-3]]:30: note: the last parameter in the range is 'IP2'
+
+void pointerConversion(int *IP, long *LP) {}
+// NO-WARN: Even though C can convert any T* to U* back and forth, compiler
+// warnings already exist for this.
+
+void testVariadicsCall() {
+  int IVal = 1;
+  decl(IVal); // NO-WARN: Particular calls to "variadics" are like template
+  // instantiations, and we do not model them.
+
+  variadic(IVal);  // NO-WARN.
+  variadic(IVal, 2, 3, 4); // NO-WARN.
+}
+
+struct S {};
+struct T {};
+
+void taggedTypes1(struct S SVar, struct T TVar) {} // NO-WARN: Distinct types.
+
+void taggedTypes2(struct S SVar1, struct S SVar2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 2 adjacent parameters of 'taggedTypes2' of similar type ('struct S')
+// CHECK-MESSAGES: :[[@LINE-2]]:28: note: the first parameter in the range is 'SVar1'
+// CHECK-MESSAGES: :[[@LINE-3]]:44: note: the last parameter in the range is 'SVar2'
+
+void wrappers(struct { int I; } I1, struct { int I; } I2) {} // NO-WARN: Distinct anonymous types.
+
+void knr(I, J)
+  int I;
+  int J;
+{}
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: 2 adjacent parameters of 'knr' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-4]]:7: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-4]]:7: note: the last parameter in the range is 'J'
+
+void boolAsWritten(bool B1, bool B2) {} // NO-WARN: The type name is ignored.
+// Note that "bool" is a macro that expands to "_Bool" 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:363
+/// Returns the diagnostic-friendly name of the node, or empty string.
+static SmallString<64> getName(const NamedDecl *ND) {
+  SmallString<64> Name;

whisperity wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > Is this still needed in light of the comments from @riccibruno?
> > It doesn't look like to me as if those patches they commented were ever 
> > merged, but I'll check it out.
> I checked at they haven't been merged yet. Nonetheless, we can remove this 
> logic later once that logic is in.
> 
> I added a test case for unnamed parameters with the current logic printing 
> ``, so if the underlying call will return something like ` parameter @ 24:48>` we will know because that test will blow up.
Thank you for checking and adding some test coverage for it!


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 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:363
+/// Returns the diagnostic-friendly name of the node, or empty string.
+static SmallString<64> getName(const NamedDecl *ND) {
+  SmallString<64> Name;

whisperity wrote:
> aaron.ballman wrote:
> > Is this still needed in light of the comments from @riccibruno?
> It doesn't look like to me as if those patches they commented were ever 
> merged, but I'll check it out.
I checked at they haven't been merged yet. Nonetheless, we can remove this 
logic later once that logic is in.

I added a test case for unnamed parameters with the current logic printing 
``, so if the underlying call will return something like `` we will know because that test will blow up.


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 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity updated this revision to Diff 331241.
whisperity marked 6 inline comments as done.
whisperity added a subscriber: Szelethus.
whisperity added a comment.

- Add `const reverse iterator` to the default-ignored typename list.


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

https://reviews.llvm.org/D69560

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
@@ -0,0 +1,148 @@
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: "bool;MyBool;struct U;MAKE_LOGICAL_TYPE(int)"} \
+// RUN:  ]}' -- -x c
+
+#define bool _Bool
+#define true 1
+#define false 0
+
+typedef bool MyBool;
+
+#define TheLogicalType bool
+
+void declVoid(void); // NO-WARN: Declaration only.
+void decl(); // NO-WARN: Declaration only.
+void oneParam(int I) {}  // NO-WARN: 1 parameter.
+void variadic(int I, ...) {} // NO-WARN: 1 visible parameter.
+
+void trivial(int I, int J) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 2 adjacent parameters of 'trivial' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:18: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:25: note: the last parameter in the range is 'J'
+
+void qualifier(int I, const int CI) {} // NO-WARN: Distinct types.
+
+void restrictQualifier(char *restrict CPR1, char *restrict CPR2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 2 adjacent parameters of 'restrictQualifier' of similar type ('char *restrict')
+// CHECK-MESSAGES: :[[@LINE-2]]:39: note: the first parameter in the range is 'CPR1'
+// CHECK-MESSAGES: :[[@LINE-3]]:60: note: the last parameter in the range is 'CPR2'
+
+void pointer1(int *IP1, int *IP2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 2 adjacent parameters of 'pointer1' of similar type ('int *')
+// CHECK-MESSAGES: :[[@LINE-2]]:20: note: the first parameter in the range is 'IP1'
+// CHECK-MESSAGES: :[[@LINE-3]]:30: note: the last parameter in the range is 'IP2'
+
+void pointerConversion(int *IP, long *LP) {}
+// NO-WARN: Even though C can convert any T* to U* back and forth, compiler
+// warnings already exist for this.
+
+void testVariadicsCall() {
+  int IVal = 1;
+  decl(IVal); // NO-WARN: Particular calls to "variadics" are like template
+  // instantiations, and we do not model them.
+
+  variadic(IVal);  // NO-WARN.
+  variadic(IVal, 2, 3, 4); // NO-WARN.
+}
+
+struct S {};
+struct T {};
+
+void taggedTypes1(struct S SVar, struct T TVar) {} // NO-WARN: Distinct types.
+
+void taggedTypes2(struct S SVar1, struct S SVar2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 2 adjacent parameters of 'taggedTypes2' of similar type ('struct S')
+// CHECK-MESSAGES: :[[@LINE-2]]:28: note: the first parameter in the range is 'SVar1'
+// CHECK-MESSAGES: :[[@LINE-3]]:44: note: the last parameter in the range is 'SVar2'
+
+void wrappers(struct { int I; } I1, struct { int I; } I2) {} // NO-WARN: Distinct anonymous types.
+
+void knr(I, J)
+  int I;
+  int J;
+{}
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: 2 adjacent parameters of 'knr' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-4]]:7: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-4]]:7: note: the last parameter in the range is 'J'
+
+void boolAsWritten(bool B1, bool B2) {} // NO-WARN: The type name is ignored.
+// Note that "bool" is a macro that expands to "_Bool" internally, but it is
+// only "bool" that is ignored from the two.
+
+void underscoreBoolAsWritten(_Bool B1, _Bool B2) {}
+// Even though it is "_Bool" that is written in the code, the diagnostic message
+// 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D69560#2629616 , @whisperity wrote:

> In D69560#2629555 , @aaron.ballman 
> wrote:
>
>> [...] so please hold off on landing it for a bit in case any of the other 
>> reviewers want to weigh in.
>
> Due to how the patch itself only being useful in practice if all the pieces 
> fall into place, I will definitely keep circling about until the **entire** 
> patch tree is ✔. (Including the filtering heuristics, etc.)

Good idea. :-)




Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:56
+
+#ifndef NDEBUG
+

whisperity wrote:
> aaron.ballman wrote:
> > whisperity wrote:
> > > aaron.ballman wrote:
> > > > Are you planning to remove the debugging code now that the check is 
> > > > approaching its final form?
> > > Actually, I would prefer not to. I removed every debug thing possible. 
> > > However, and this is speaking from experience because I wrote this check 
> > > two times already from basically scratch... the rest of the debug code 
> > > that is part of the patch has to be there. If anything goes nuts, 
> > > especially if there would be false positives later... it would be 
> > > impossible to figure out what is going on during the modelling without 
> > > seeing all the steps being taken.
> > We don't often leave debugging statements in because they tend to cause a 
> > maintenance burden that doesn't justify their benefit. However, I agree 
> > with your observation that this code is quite difficult to reason through 
> > without debugging aids.
> > 
> > I don't insist on removing the code yet, but would say we should remain 
> > open to the idea if it causes a burden in practice. (Either in terms of 
> > keeping the debugging code up to date as the check evolves, but also in 
> > terms of compile time performance for the check if the debugging code paths 
> > turn out to be expensive on build times.)
> All debugging code is wrapped into the `LLVM_DEBUG` macro specifically 
> (that's why I even put this little "print bits" function behind `NDEBUG`) so 
> they are not part of the builds.
> 
> I think in general //if// someone puts the effort into reading the code and 
> has an interactive debugger they could figure it out (especially if they keep 
> track of the recursion constantly), I tried to make everything nicely padded 
> and all the variable names and control flow to make sense. (Of course the 
> wealth of complexity comes in the follow-up patches.)
> All debugging code is wrapped into the LLVM_DEBUG macro specifically (that's 
> why I even put this little "print bits" function behind NDEBUG) so they are 
> not part of the builds.

They're part of debug builds still (which is the build configuration I tend to 
use most often). That said, I think it's fine to leave the debugging code in 
for now, especially as the check is being actively worked on.


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 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D69560#2629555 , @aaron.ballman 
wrote:

> [...] so please hold off on landing it for a bit in case any of the other 
> reviewers want to weigh in.

Due to how the patch itself only being useful in practice if all the pieces 
fall into place, I will definitely keep circling about until the **entire** 
patch tree is ✔. (Including the filtering heuristics, etc.)




Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:56
+
+#ifndef NDEBUG
+

aaron.ballman wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > Are you planning to remove the debugging code now that the check is 
> > > approaching its final form?
> > Actually, I would prefer not to. I removed every debug thing possible. 
> > However, and this is speaking from experience because I wrote this check 
> > two times already from basically scratch... the rest of the debug code that 
> > is part of the patch has to be there. If anything goes nuts, especially if 
> > there would be false positives later... it would be impossible to figure 
> > out what is going on during the modelling without seeing all the steps 
> > being taken.
> We don't often leave debugging statements in because they tend to cause a 
> maintenance burden that doesn't justify their benefit. However, I agree with 
> your observation that this code is quite difficult to reason through without 
> debugging aids.
> 
> I don't insist on removing the code yet, but would say we should remain open 
> to the idea if it causes a burden in practice. (Either in terms of keeping 
> the debugging code up to date as the check evolves, but also in terms of 
> compile time performance for the check if the debugging code paths turn out 
> to be expensive on build times.)
All debugging code is wrapped into the `LLVM_DEBUG` macro specifically (that's 
why I even put this little "print bits" function behind `NDEBUG`) so they are 
not part of the builds.

I think in general //if// someone puts the effort into reading the code and has 
an interactive debugger they could figure it out (especially if they keep track 
of the recursion constantly), I tried to make everything nicely padded and all 
the variable names and control flow to make sense. (Of course the wealth of 
complexity comes in the follow-up patches.)


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 'bugprone-easily-swappable-parameters' check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

In general, I'm happy with this check (and the follow-ups), so I'm giving it my 
LG. However, it's complex enough that I think having some extra review from 
@alexfh, @hokein, or one of the other reviewers would be a good idea, so please 
hold off on landing it for a bit in case any of the other reviewers want to 
weigh in.




Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:56
+
+#ifndef NDEBUG
+

whisperity wrote:
> aaron.ballman wrote:
> > Are you planning to remove the debugging code now that the check is 
> > approaching its final form?
> Actually, I would prefer not to. I removed every debug thing possible. 
> However, and this is speaking from experience because I wrote this check two 
> times already from basically scratch... the rest of the debug code that is 
> part of the patch has to be there. If anything goes nuts, especially if there 
> would be false positives later... it would be impossible to figure out what 
> is going on during the modelling without seeing all the steps being taken.
We don't often leave debugging statements in because they tend to cause a 
maintenance burden that doesn't justify their benefit. However, I agree with 
your observation that this code is quite difficult to reason through without 
debugging aids.

I don't insist on removing the code yet, but would say we should remain open to 
the idea if it causes a burden in practice. (Either in terms of keeping the 
debugging code up to date as the check evolves, but also in terms of compile 
time performance for the check if the debugging code paths turn out to be 
expensive on build times.)


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 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:363
+/// Returns the diagnostic-friendly name of the node, or empty string.
+static SmallString<64> getName(const NamedDecl *ND) {
+  SmallString<64> Name;

aaron.ballman wrote:
> Is this still needed in light of the comments from @riccibruno?
It doesn't look like to me as if those patches they commented were ever merged, 
but I'll check it out.


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 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:56
+
+#ifndef NDEBUG
+

aaron.ballman wrote:
> Are you planning to remove the debugging code now that the check is 
> approaching its final form?
Actually, I would prefer not to. I removed every debug thing possible. However, 
and this is speaking from experience because I wrote this check two times 
already from basically scratch... the rest of the debug code that is part of 
the patch has to be there. If anything goes nuts, especially if there would be 
false positives later... it would be impossible to figure out what is going on 
during the modelling without seeing all the steps being taken.


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 'bugprone-easily-swappable-parameters' check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:54
+   "reverse_iterator",
+   "reverse_const_iterator"});
+

`const_reverse_iterator` seems plausible here for the same reason as 
`const_iterator`.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:56
+
+#ifndef NDEBUG
+

Are you planning to remove the debugging code now that the check is approaching 
its final form?



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:363
+/// Returns the diagnostic-friendly name of the node, or empty string.
+static SmallString<64> getName(const NamedDecl *ND) {
+  SmallString<64> Name;

Is this still needed in light of the comments from @riccibruno?


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 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

@aaron.ballman I've reworked and uploaded the final addition to this line, the 
crown jewel, the very reason this checker was developed (D75041 
). So, AFAIK, the "let's rewrite the whole 
thing with better code quality" work can be considered done, and we may move on 
with the review.


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 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

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

- **[NFC]** Reformat the documentation so option values are in single backticks.


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/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
@@ -0,0 +1,148 @@
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: "bool;MyBool;struct U;MAKE_LOGICAL_TYPE(int)"} \
+// RUN:  ]}' -- -x c
+
+#define bool _Bool
+#define true 1
+#define false 0
+
+typedef bool MyBool;
+
+#define TheLogicalType bool
+
+void declVoid(void); // NO-WARN: Declaration only.
+void decl(); // NO-WARN: Declaration only.
+void oneParam(int I) {}  // NO-WARN: 1 parameter.
+void variadic(int I, ...) {} // NO-WARN: 1 visible parameter.
+
+void trivial(int I, int J) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 2 adjacent parameters of 'trivial' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:18: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:25: note: the last parameter in the range is 'J'
+
+void qualifier(int I, const int CI) {} // NO-WARN: Distinct types.
+
+void restrictQualifier(char *restrict CPR1, char *restrict CPR2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 2 adjacent parameters of 'restrictQualifier' of similar type ('char *restrict')
+// CHECK-MESSAGES: :[[@LINE-2]]:39: note: the first parameter in the range is 'CPR1'
+// CHECK-MESSAGES: :[[@LINE-3]]:60: note: the last parameter in the range is 'CPR2'
+
+void pointer1(int *IP1, int *IP2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 2 adjacent parameters of 'pointer1' of similar type ('int *')
+// CHECK-MESSAGES: :[[@LINE-2]]:20: note: the first parameter in the range is 'IP1'
+// CHECK-MESSAGES: :[[@LINE-3]]:30: note: the last parameter in the range is 'IP2'
+
+void pointerConversion(int *IP, long *LP) {}
+// NO-WARN: Even though C can convert any T* to U* back and forth, compiler
+// warnings already exist for this.
+
+void testVariadicsCall() {
+  int IVal = 1;
+  decl(IVal); // NO-WARN: Particular calls to "variadics" are like template
+  // instantiations, and we do not model them.
+
+  variadic(IVal);  // NO-WARN.
+  variadic(IVal, 2, 3, 4); // NO-WARN.
+}
+
+struct S {};
+struct T {};
+
+void taggedTypes1(struct S SVar, struct T TVar) {} // NO-WARN: Distinct types.
+
+void taggedTypes2(struct S SVar1, struct S SVar2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 2 adjacent parameters of 'taggedTypes2' of similar type ('struct S')
+// CHECK-MESSAGES: :[[@LINE-2]]:28: note: the first parameter in the range is 'SVar1'
+// CHECK-MESSAGES: :[[@LINE-3]]:44: note: the last parameter in the range is 'SVar2'
+
+void wrappers(struct { int I; } I1, struct { int I; } I2) {} // NO-WARN: Distinct anonymous types.
+
+void knr(I, J)
+  int I;
+  int J;
+{}
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: 2 adjacent parameters of 'knr' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-4]]:7: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-4]]:7: note: the last parameter in the range is 'J'
+
+void boolAsWritten(bool B1, bool B2) {} // NO-WARN: The type name is ignored.
+// Note that "bool" is a macro that expands to "_Bool" internally, but it is
+// only "bool" that is ignored from the two.
+
+void underscoreBoolAsWritten(_Bool B1, _Bool B2) {}
+// Even though it is "_Bool" that is written in the code, the diagnostic message
+// respects the printing policy as defined 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

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

Make sure that overloadable binary operators are ignored properly.


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/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
@@ -0,0 +1,148 @@
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: "bool;MyBool;struct U;MAKE_LOGICAL_TYPE(int)"} \
+// RUN:  ]}' -- -x c
+
+#define bool _Bool
+#define true 1
+#define false 0
+
+typedef bool MyBool;
+
+#define TheLogicalType bool
+
+void declVoid(void); // NO-WARN: Declaration only.
+void decl(); // NO-WARN: Declaration only.
+void oneParam(int I) {}  // NO-WARN: 1 parameter.
+void variadic(int I, ...) {} // NO-WARN: 1 visible parameter.
+
+void trivial(int I, int J) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 2 adjacent parameters of 'trivial' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:18: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:25: note: the last parameter in the range is 'J'
+
+void qualifier(int I, const int CI) {} // NO-WARN: Distinct types.
+
+void restrictQualifier(char *restrict CPR1, char *restrict CPR2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 2 adjacent parameters of 'restrictQualifier' of similar type ('char *restrict')
+// CHECK-MESSAGES: :[[@LINE-2]]:39: note: the first parameter in the range is 'CPR1'
+// CHECK-MESSAGES: :[[@LINE-3]]:60: note: the last parameter in the range is 'CPR2'
+
+void pointer1(int *IP1, int *IP2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 2 adjacent parameters of 'pointer1' of similar type ('int *')
+// CHECK-MESSAGES: :[[@LINE-2]]:20: note: the first parameter in the range is 'IP1'
+// CHECK-MESSAGES: :[[@LINE-3]]:30: note: the last parameter in the range is 'IP2'
+
+void pointerConversion(int *IP, long *LP) {}
+// NO-WARN: Even though C can convert any T* to U* back and forth, compiler
+// warnings already exist for this.
+
+void testVariadicsCall() {
+  int IVal = 1;
+  decl(IVal); // NO-WARN: Particular calls to "variadics" are like template
+  // instantiations, and we do not model them.
+
+  variadic(IVal);  // NO-WARN.
+  variadic(IVal, 2, 3, 4); // NO-WARN.
+}
+
+struct S {};
+struct T {};
+
+void taggedTypes1(struct S SVar, struct T TVar) {} // NO-WARN: Distinct types.
+
+void taggedTypes2(struct S SVar1, struct S SVar2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 2 adjacent parameters of 'taggedTypes2' of similar type ('struct S')
+// CHECK-MESSAGES: :[[@LINE-2]]:28: note: the first parameter in the range is 'SVar1'
+// CHECK-MESSAGES: :[[@LINE-3]]:44: note: the last parameter in the range is 'SVar2'
+
+void wrappers(struct { int I; } I1, struct { int I; } I2) {} // NO-WARN: Distinct anonymous types.
+
+void knr(I, J)
+  int I;
+  int J;
+{}
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: 2 adjacent parameters of 'knr' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-4]]:7: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-4]]:7: note: the last parameter in the range is 'J'
+
+void boolAsWritten(bool B1, bool B2) {} // NO-WARN: The type name is ignored.
+// Note that "bool" is a macro that expands to "_Bool" internally, but it is
+// only "bool" that is ignored from the two.
+
+void underscoreBoolAsWritten(_Bool B1, _Bool B2) {}
+// Even though it is "_Bool" that is written in the code, the diagnostic message
+// respects the printing policy as defined by the 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

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

**NFC** (With all the restructuring and trying to figure out how to implement 
the latest change, I managed to upload the wrong, unclean diff...)


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/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
@@ -0,0 +1,148 @@
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: "bool;MyBool;struct U;MAKE_LOGICAL_TYPE(int)"} \
+// RUN:  ]}' -- -x c
+
+#define bool _Bool
+#define true 1
+#define false 0
+
+typedef bool MyBool;
+
+#define TheLogicalType bool
+
+void declVoid(void); // NO-WARN: Declaration only.
+void decl(); // NO-WARN: Declaration only.
+void oneParam(int I) {}  // NO-WARN: 1 parameter.
+void variadic(int I, ...) {} // NO-WARN: 1 visible parameter.
+
+void trivial(int I, int J) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 2 adjacent parameters of 'trivial' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:18: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:25: note: the last parameter in the range is 'J'
+
+void qualifier(int I, const int CI) {} // NO-WARN: Distinct types.
+
+void restrictQualifier(char *restrict CPR1, char *restrict CPR2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 2 adjacent parameters of 'restrictQualifier' of similar type ('char *restrict')
+// CHECK-MESSAGES: :[[@LINE-2]]:39: note: the first parameter in the range is 'CPR1'
+// CHECK-MESSAGES: :[[@LINE-3]]:60: note: the last parameter in the range is 'CPR2'
+
+void pointer1(int *IP1, int *IP2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 2 adjacent parameters of 'pointer1' of similar type ('int *')
+// CHECK-MESSAGES: :[[@LINE-2]]:20: note: the first parameter in the range is 'IP1'
+// CHECK-MESSAGES: :[[@LINE-3]]:30: note: the last parameter in the range is 'IP2'
+
+void pointerConversion(int *IP, long *LP) {}
+// NO-WARN: Even though C can convert any T* to U* back and forth, compiler
+// warnings already exist for this.
+
+void testVariadicsCall() {
+  int IVal = 1;
+  decl(IVal); // NO-WARN: Particular calls to "variadics" are like template
+  // instantiations, and we do not model them.
+
+  variadic(IVal);  // NO-WARN.
+  variadic(IVal, 2, 3, 4); // NO-WARN.
+}
+
+struct S {};
+struct T {};
+
+void taggedTypes1(struct S SVar, struct T TVar) {} // NO-WARN: Distinct types.
+
+void taggedTypes2(struct S SVar1, struct S SVar2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 2 adjacent parameters of 'taggedTypes2' of similar type ('struct S')
+// CHECK-MESSAGES: :[[@LINE-2]]:28: note: the first parameter in the range is 'SVar1'
+// CHECK-MESSAGES: :[[@LINE-3]]:44: note: the last parameter in the range is 'SVar2'
+
+void wrappers(struct { int I; } I1, struct { int I; } I2) {} // NO-WARN: Distinct anonymous types.
+
+void knr(I, J)
+  int I;
+  int J;
+{}
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: 2 adjacent parameters of 'knr' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-4]]:7: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-4]]:7: note: the last parameter in the range is 'J'
+
+void boolAsWritten(bool B1, bool B2) {} // NO-WARN: The type name is ignored.
+// Note that "bool" is a macro that expands to "_Bool" internally, but it is
+// only "bool" that is ignored from the two.
+
+void underscoreBoolAsWritten(_Bool B1, _Bool B2) {}
+// Even though it is "_Bool" that is written in the code, 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:255
+  std::string NodeTypeName =
+  Node->getType().getAsString(Node->getASTContext().getPrintingPolicy());
+  StringRef CaptureName{NodeTypeName};

whisperity wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > whisperity wrote:
> > > > aaron.ballman wrote:
> > > > > Hmm, one downside to using the printing policy to get the node name 
> > > > > is that there can be alternative spellings for the same type that the 
> > > > > user might reasonably expect to be applied. e.g., if the user expects 
> > > > > to ignore `bool` datatypes and the printing policy wants to spell the 
> > > > > type as `_Bool`, this won't behave as expected.
> > > > But then the diagnostic is inconsistent with what the compiler is 
> > > > configured to output as diagnostics, isn't it? Can I stringify through 
> > > > Clang with some "default" printing policy?
> > > The situation I'm worried about is something like this in C code:
> > > ```
> > > #include 
> > > 
> > > void func(bool b, bool c) {}
> > > ```
> > > because the `bool` in the function signature will expand to `_Bool` after 
> > > the preprocessor gets to it (because of the contents of ``). 
> > > If the user writes `bool` in their configuration file because that's what 
> > > they've written in their function signatures, will this fail because the 
> > > printing policy says it should be `_Bool`?
> > > 
> > > Along similar lines, I wonder about things like `struct S` vs `S` (in C), 
> > > `signed char` vs `char`, types within anonymous namespaces (where we 
> > > print ``), etc. Basically, any time when the 
> > > printing policy may differ from what the user lexically writes in code.
> > Meanwhile, I realised we are talking of two entirely distinct things. I 
> > will look into how this `PrintingPolicy` stuff works... I agree that the 
> > ignore rules (where the comment was originally posted) should respect the 
> > text written into the code as-is. The diagnostics printing type names could 
> > continue using the proper printing policy (to be in line with how Sema 
> > diagnoses, AFAIK).
> Right, @aaron.ballman I think I got this tested. Please check the C test 
> file. `bool` becomes `_Bool` in C code by the preprocessor, but the printing 
> policy still says both should be printed as `bool`. I added some test cases 
> about this. It is weird, because without doing some extra magic specifically 
> against macros (which might break detecting type names in other locations), I 
> can't really test ignoring `_Bool` but not ignoring `bool`. Now, the 
> diagnostics respect the printing policy, but the type names are taken with an 
> empty printing policy, which //should// give us the result as-is in the code. 
> Unfortunately, "as-is" still means `_Bool` for `bool`'s case, so even if you 
> ignore `bool`, you will still get results where `bool` is written, because 
> `bool` is a macro (Tidy automatically puts a note about this!) and the real 
> type is `_Bool`, as per the macro expansion.
> 
> Wow... okay, that explanation got convoluted.
> Anyways, it's //really weird//. But: we no longer take the actual printing 
> policy anymore, but the default, which should eliminate all these cases.
> The `bool`/`_Bool` case doesn't fail because of the printing policy, it fails 
> because `bool` is a macro in ``.
> 
> Also, the type name ignore list not a full match, but a //substring match// 
> ignore list. So if you have a type named `struct Foo`, if you ignore `Foo`, 
> both `struct Foo`, `Foo`, `MyFoo`, etc. will be also ignored. And it's case 
> sensitive.
Nevermind, I think I figured this out... or at least managed to figure it out 
partially... This "SpellingLoc" and "ExpansionLoc" and all this stuff is really 
convoluted, and there doesn't seem to be any good documentation for them.


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 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

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

Made detection of macros as "parameter type names" more robust.


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/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
@@ -0,0 +1,148 @@
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: "bool;MyBool;struct U;MAKE_LOGICAL_TYPE(int)"} \
+// RUN:  ]}' -- -x c
+
+#define bool _Bool
+#define true 1
+#define false 0
+
+typedef bool MyBool;
+
+#define TheLogicalType bool
+
+void declVoid(void); // NO-WARN: Declaration only.
+void decl(); // NO-WARN: Declaration only.
+void oneParam(int I) {}  // NO-WARN: 1 parameter.
+void variadic(int I, ...) {} // NO-WARN: 1 visible parameter.
+
+void trivial(int I, int J) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 2 adjacent parameters of 'trivial' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:18: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:25: note: the last parameter in the range is 'J'
+
+void qualifier(int I, const int CI) {} // NO-WARN: Distinct types.
+
+void restrictQualifier(char *restrict CPR1, char *restrict CPR2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 2 adjacent parameters of 'restrictQualifier' of similar type ('char *restrict')
+// CHECK-MESSAGES: :[[@LINE-2]]:39: note: the first parameter in the range is 'CPR1'
+// CHECK-MESSAGES: :[[@LINE-3]]:60: note: the last parameter in the range is 'CPR2'
+
+void pointer1(int *IP1, int *IP2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 2 adjacent parameters of 'pointer1' of similar type ('int *')
+// CHECK-MESSAGES: :[[@LINE-2]]:20: note: the first parameter in the range is 'IP1'
+// CHECK-MESSAGES: :[[@LINE-3]]:30: note: the last parameter in the range is 'IP2'
+
+void pointerConversion(int *IP, long *LP) {}
+// NO-WARN: Even though C can convert any T* to U* back and forth, compiler
+// warnings already exist for this.
+
+void testVariadicsCall() {
+  int IVal = 1;
+  decl(IVal); // NO-WARN: Particular calls to "variadics" are like template
+  // instantiations, and we do not model them.
+
+  variadic(IVal);  // NO-WARN.
+  variadic(IVal, 2, 3, 4); // NO-WARN.
+}
+
+struct S {};
+struct T {};
+
+void taggedTypes1(struct S SVar, struct T TVar) {} // NO-WARN: Distinct types.
+
+void taggedTypes2(struct S SVar1, struct S SVar2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 2 adjacent parameters of 'taggedTypes2' of similar type ('struct S')
+// CHECK-MESSAGES: :[[@LINE-2]]:28: note: the first parameter in the range is 'SVar1'
+// CHECK-MESSAGES: :[[@LINE-3]]:44: note: the last parameter in the range is 'SVar2'
+
+void wrappers(struct { int I; } I1, struct { int I; } I2) {} // NO-WARN: Distinct anonymous types.
+
+void knr(I, J)
+  int I;
+  int J;
+{}
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: 2 adjacent parameters of 'knr' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-4]]:7: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-4]]:7: note: the last parameter in the range is 'J'
+
+void boolAsWritten(bool B1, bool B2) {} // NO-WARN: The type name is ignored.
+// Note that "bool" is a macro that expands to "_Bool" internally, but it is
+// only "bool" that is ignored from the two.
+
+void underscoreBoolAsWritten(_Bool B1, _Bool B2) {}
+// Even though it is "_Bool" that is written in the code, the diagnostic message
+// respects the printing policy as defined by the 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity marked 4 inline comments as done.
whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:255
+  std::string NodeTypeName =
+  Node->getType().getAsString(Node->getASTContext().getPrintingPolicy());
+  StringRef CaptureName{NodeTypeName};

whisperity wrote:
> aaron.ballman wrote:
> > whisperity wrote:
> > > aaron.ballman wrote:
> > > > Hmm, one downside to using the printing policy to get the node name is 
> > > > that there can be alternative spellings for the same type that the user 
> > > > might reasonably expect to be applied. e.g., if the user expects to 
> > > > ignore `bool` datatypes and the printing policy wants to spell the type 
> > > > as `_Bool`, this won't behave as expected.
> > > But then the diagnostic is inconsistent with what the compiler is 
> > > configured to output as diagnostics, isn't it? Can I stringify through 
> > > Clang with some "default" printing policy?
> > The situation I'm worried about is something like this in C code:
> > ```
> > #include 
> > 
> > void func(bool b, bool c) {}
> > ```
> > because the `bool` in the function signature will expand to `_Bool` after 
> > the preprocessor gets to it (because of the contents of ``). If 
> > the user writes `bool` in their configuration file because that's what 
> > they've written in their function signatures, will this fail because the 
> > printing policy says it should be `_Bool`?
> > 
> > Along similar lines, I wonder about things like `struct S` vs `S` (in C), 
> > `signed char` vs `char`, types within anonymous namespaces (where we print 
> > ``), etc. Basically, any time when the printing policy 
> > may differ from what the user lexically writes in code.
> Meanwhile, I realised we are talking of two entirely distinct things. I will 
> look into how this `PrintingPolicy` stuff works... I agree that the ignore 
> rules (where the comment was originally posted) should respect the text 
> written into the code as-is. The diagnostics printing type names could 
> continue using the proper printing policy (to be in line with how Sema 
> diagnoses, AFAIK).
Right, @aaron.ballman I think I got this tested. Please check the C test file. 
`bool` becomes `_Bool` in C code by the preprocessor, but the printing policy 
still says both should be printed as `bool`. I added some test cases about 
this. It is weird, because without doing some extra magic specifically against 
macros (which might break detecting type names in other locations), I can't 
really test ignoring `_Bool` but not ignoring `bool`. Now, the diagnostics 
respect the printing policy, but the type names are taken with an empty 
printing policy, which //should// give us the result as-is in the code. 
Unfortunately, "as-is" still means `_Bool` for `bool`'s case, so even if you 
ignore `bool`, you will still get results where `bool` is written, because 
`bool` is a macro (Tidy automatically puts a note about this!) and the real 
type is `_Bool`, as per the macro expansion.

Wow... okay, that explanation got convoluted.
Anyways, it's //really weird//. But: we no longer take the actual printing 
policy anymore, but the default, which should eliminate all these cases.
The `bool`/`_Bool` case doesn't fail because of the printing policy, it fails 
because `bool` is a macro in ``.

Also, the type name ignore list not a full match, but a //substring match// 
ignore list. So if you have a type named `struct Foo`, if you ignore `Foo`, 
both `struct Foo`, `Foo`, `MyFoo`, etc. will be also ignored. And it's case 
sensitive.


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 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

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

- Added a C test file with some C-specific language constructs that we may or 
may not match.
- Explained limitations with regards to variadics, templates, etc. better in 
the docs, and added test cases for these too.
- When comparing the type names against the ignored type name list, do **not** 
consider `PrintingPolicy`, but take //exactly// what's in the source code. This 
mostly concerns `bool` and `_Bool` at face value, so this is tested in the C 
file too.


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/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
@@ -0,0 +1,93 @@
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: "bool;MyBool;struct U"} \
+// RUN:  ]}' -- -x c
+
+#define bool _Bool
+#define true 1
+#define false 0
+
+typedef bool MyBool;
+
+void declVoid(void); // NO-WARN: Declaration only.
+void decl(); // NO-WARN: Declaration only.
+void oneParam(int I) {}  // NO-WARN: 1 parameter.
+void variadic(int I, ...) {} // NO-WARN: 1 visible parameter.
+
+void trivial(int I, int J) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 2 adjacent parameters of 'trivial' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:18: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:25: note: the last parameter in the range is 'J'
+
+void qualifier(int I, const int CI) {} // NO-WARN: Distinct types.
+
+void restrictQualifier(char *restrict CPR1, char *restrict CPR2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 2 adjacent parameters of 'restrictQualifier' of similar type ('char *restrict')
+// CHECK-MESSAGES: :[[@LINE-2]]:39: note: the first parameter in the range is 'CPR1'
+// CHECK-MESSAGES: :[[@LINE-3]]:60: note: the last parameter in the range is 'CPR2'
+
+void pointer1(int *IP1, int *IP2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 2 adjacent parameters of 'pointer1' of similar type ('int *')
+// CHECK-MESSAGES: :[[@LINE-2]]:20: note: the first parameter in the range is 'IP1'
+// CHECK-MESSAGES: :[[@LINE-3]]:30: note: the last parameter in the range is 'IP2'
+
+void pointerConversion(int *IP, long *LP) {}
+// NO-WARN: Even though C can convert any T* to U* back and forth, compiler
+// warnings already exist for this.
+
+void testVariadicsCall() {
+  int IVal = 1;
+  decl(IVal); // NO-WARN: Particular calls to "variadics" are like template
+  // instantiations, and we do not model them.
+
+  variadic(IVal);  // NO-WARN.
+  variadic(IVal, 2, 3, 4); // NO-WARN.
+}
+
+struct S {};
+struct T {};
+
+void taggedTypes1(struct S SVar, struct T TVar) {} // NO-WARN: Distinct types.
+
+void taggedTypes2(struct S SVar1, struct S SVar2) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: 2 adjacent parameters of 'taggedTypes2' of similar type ('struct S')
+// CHECK-MESSAGES: :[[@LINE-2]]:28: note: the first parameter in the range is 'SVar1'
+// CHECK-MESSAGES: :[[@LINE-3]]:44: note: the last parameter in the range is 'SVar2'
+
+void unholy(I, J)
+  int I;
+  int J;
+{}
+// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: 2 adjacent parameters of 'unholy' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-4]]:7: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-4]]:7: note: the last parameter in the range is 'J'
+
+void boolAsWritten(bool B1, bool B2) {}
+// Even though it is "bool" what is written there, the preprocessor has changed
+// it to "_Bool" due to 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D69560#2549959 , @steveire wrote:

> Why "easily" instead of "suspicious", "spuriously" or any of the other words 
> that are already used?

The name was @aaron.ballman's idea after we postponed the 
`experimental-cppcoreguidelines-yaddayaddayadda...`.
The best I could come up with is `potentially`. The `suspicious` is a problem 
because this check is an interface check, not a call-site check. (The sister 
check is currently called `suspiciously-swapped-argument` or something like 
that.)

The original check idea from C++ Core Guidelines 

 also uses the phrase:

> Reason: Adjacent arguments of the same type are easily swapped by mistake.

However, `potentially-swappable-parameters` just sounds... wonky. It does not 
carry the weight the rule otherwise meant to.
`weakly-typed-parameters`?

I am not good with names, sadly.



I am conflicted about saying that "easily" **always** refers to some "positive 
aspect", however. Use a [[unnamed trademarked safety device]] because you can 
end up easily cutting off your finger during work. Do not touch a stove/hot 
water fountain, because you can easily get burnt. Leaving Christmas candles 
unattended could easily lead to a house fire. Does anyone want to get hurt, 
burnt, their house destroyed, etc.? There is nothing "positive" in these 
suggestions or regulations either. And we are doing a tool that is practically 
the same thing, trying to prevent a (different kind of) fire.


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 'bugprone-easily-swappable-parameters' check

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D69560#2536597 , @whisperity wrote:

> In D69560#2536570 , @steveire wrote:
>
>> I haven't read through all the comments, but the word 'easily' implies 
>> 'desirable'. This check seems to be for finding params which are undesirably 
>> swappable, right?
>
> The `easily` was to mean that the swap can be done with little effort (i.e. 
> "in an easy fashion"?) and by accident.

I understand that. The problem is the name. The `signal-to-kill-thread` check 
is `bad`. The `erase` check is `inaccurate`. The `roundings` check is 
`incorrect`. The `operator-in-strlen-in-alloc` check is `misplaced`. Those are 
all words that indicate the negative. Your name indicates the positive 
("easily"). The name indicates that the swappable params are desirable.

Why "easily" instead of "suspicious", "spuriously" or any of the other words 
that are already used?


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 'bugprone-easily-swappable-parameters' check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:177
+   const std::size_t StartIndex) {
+  const std::size_t NumParams = FD->getNumParams();
+  assert(StartIndex < NumParams && "out of bounds for start");

whisperity wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > whisperity wrote:
> > > > aaron.ballman wrote:
> > > > > Some interesting test cases to consider: varargs functions and K C 
> > > > > functions
> > > > > K C functions
> > > > 
> > > > Call me too young, but I had to look up what a "K C function" is, and 
> > > > I am absolutely baffled how this unholy construct is still supported! 
> > > > **But:** thanks to Clang supporting it properly in the AST, the checker 
> > > > works out of the box!
> > > > 
> > > > Given
> > > > 
> > > > ```
> > > > int foo(a, b)
> > > >   int a;
> > > >   int b;
> > > > {
> > > >   return a + b;
> > > > }
> > > > ```
> > > > 
> > > > We get the following output:
> > > > 
> > > > ```
> > > > /tmp/knr.c:2:3: warning: 2 adjacent parameters of 'foo' of similar type 
> > > > ('int') are easily swapped by mistake 
> > > > [bugprone-easily-swappable-parameters]
> > > >   int a;
> > > >   ^
> > > > /tmp/knr.c:2:7: note: the first parameter in the range is 'a'
> > > >   int a;
> > > >   ^
> > > > /tmp/knr.c:3:7: note: the last parameter in the range is 'b'
> > > >   int b;
> > > >   ^
> > > > ```
> > > > 
> > > > (even the locations are consistent!)
> > > > 
> > > > Should I add a test case for this? We could use a specifically C test 
> > > > case either way eventually...
> > > > 
> > > > -
> > > > 
> > > > > varargs functions
> > > > 
> > > > This is a bit of terminology, but something tells me you meant the 
> > > > //variadic function// here, right? As opposed to type parameter packs.
> > > > 
> > > > Given
> > > > 
> > > > ```
> > > > int sum(int ints...) {
> > > >   return 0;
> > > > }
> > > > ```
> > > > 
> > > > the AST looks something like this:
> > > > 
> > > > ```
> > > > `-FunctionDecl 0x56372e29e258  line:1:5 sum 
> > > > 'int (int, ...)'
> > > >   |-ParmVarDecl 0x56372e29e188  col:13 ints 'int'
> > > > ```
> > > > 
> > > > Should we diagnose this? And if so, how? The variadic part is not 
> > > > represented (at least not at first glance?) in the AST. Understanding 
> > > > the insides of such a function would require either overapproximatic 
> > > > stuff and doing a looot of extra handling, or becoming flow 
> > > > sensitive... and we'd still need to understand all the `va_` standard 
> > > > functions' semantics either way.
> > > > Call me too young, but I had to look up what a "K C function" is, and 
> > > > I am absolutely baffled how this unholy construct is still supported!
> > > 
> > > Ah, to be innocent again, how I miss those days. :-D
> > > 
> > > > Should I add a test case for this? We could use a specifically C test 
> > > > case either way eventually...
> > > 
> > > I think it'd be a useful case, but the one I was specifically more 
> > > concerned with is:
> > > ```
> > > // N.B.: this is C-specific and does not apply to C++.
> > > void f();
> > > 
> > > int main(void) {
> > >   f(1, 2, 3.4, "this is why we can't have nice things");
> > > }
> > > ```
> > > where the function has no prototype and so is treated as a varargs call.
> > > 
> > > > This is a bit of terminology, but something tells me you meant the 
> > > > variadic function here, right? As opposed to type parameter packs.
> > > 
> > > Yes, sorry for being unclear, I am talking about variadic functions.
> > > 
> > > > Should we diagnose this? And if so, how? The variadic part is not 
> > > > represented (at least not at first glance?) in the AST. Understanding 
> > > > the insides of such a function would require either overapproximatic 
> > > > stuff and doing a looot of extra handling, or becoming flow 
> > > > sensitive... and we'd still need to understand all the va_ standard 
> > > > functions' semantics either way.
> > > 
> > > Well, that's what I'm wondering, really. The arguments are certainly easy 
> > > to swap because the type system can't help the user to identify swaps 
> > > without further information (like format specifier strings). However, the 
> > > checking code would be... highly unpleasant, I suspect. My intuition is 
> > > to say that we don't support functions without prototypes at all (we just 
> > > silently ignore them) and that we only check the typed parameters in a 
> > > variadic function declaration (e.g., we'll diagnose `void foo(int i, int 
> > > j, ...);` because of the sequential `int` parameters, but we won't 
> > > diagnose `void foo(int i, ...);` even if call sites look like `foo(1, 
> > > 2);`). WDYT?
> > It is definitely highly unpleasant, at one point I remember just glancing 
> > into the logic behind `printf()` related warnings in Sema and it was... 
> > odd, to say the least.
> 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:177
+   const std::size_t StartIndex) {
+  const std::size_t NumParams = FD->getNumParams();
+  assert(StartIndex < NumParams && "out of bounds for start");

aaron.ballman wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > Some interesting test cases to consider: varargs functions and K C 
> > > functions
> > > K C functions
> > 
> > Call me too young, but I had to look up what a "K C function" is, and I 
> > am absolutely baffled how this unholy construct is still supported! 
> > **But:** thanks to Clang supporting it properly in the AST, the checker 
> > works out of the box!
> > 
> > Given
> > 
> > ```
> > int foo(a, b)
> >   int a;
> >   int b;
> > {
> >   return a + b;
> > }
> > ```
> > 
> > We get the following output:
> > 
> > ```
> > /tmp/knr.c:2:3: warning: 2 adjacent parameters of 'foo' of similar type 
> > ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
> >   int a;
> >   ^
> > /tmp/knr.c:2:7: note: the first parameter in the range is 'a'
> >   int a;
> >   ^
> > /tmp/knr.c:3:7: note: the last parameter in the range is 'b'
> >   int b;
> >   ^
> > ```
> > 
> > (even the locations are consistent!)
> > 
> > Should I add a test case for this? We could use a specifically C test case 
> > either way eventually...
> > 
> > -
> > 
> > > varargs functions
> > 
> > This is a bit of terminology, but something tells me you meant the 
> > //variadic function// here, right? As opposed to type parameter packs.
> > 
> > Given
> > 
> > ```
> > int sum(int ints...) {
> >   return 0;
> > }
> > ```
> > 
> > the AST looks something like this:
> > 
> > ```
> > `-FunctionDecl 0x56372e29e258  line:1:5 sum 
> > 'int (int, ...)'
> >   |-ParmVarDecl 0x56372e29e188  col:13 ints 'int'
> > ```
> > 
> > Should we diagnose this? And if so, how? The variadic part is not 
> > represented (at least not at first glance?) in the AST. Understanding the 
> > insides of such a function would require either overapproximatic stuff and 
> > doing a looot of extra handling, or becoming flow sensitive... and we'd 
> > still need to understand all the `va_` standard functions' semantics either 
> > way.
> > Call me too young, but I had to look up what a "K C function" is, and I 
> > am absolutely baffled how this unholy construct is still supported!
> 
> Ah, to be innocent again, how I miss those days. :-D
> 
> > Should I add a test case for this? We could use a specifically C test case 
> > either way eventually...
> 
> I think it'd be a useful case, but the one I was specifically more concerned 
> with is:
> ```
> // N.B.: this is C-specific and does not apply to C++.
> void f();
> 
> int main(void) {
>   f(1, 2, 3.4, "this is why we can't have nice things");
> }
> ```
> where the function has no prototype and so is treated as a varargs call.
> 
> > This is a bit of terminology, but something tells me you meant the variadic 
> > function here, right? As opposed to type parameter packs.
> 
> Yes, sorry for being unclear, I am talking about variadic functions.
> 
> > Should we diagnose this? And if so, how? The variadic part is not 
> > represented (at least not at first glance?) in the AST. Understanding the 
> > insides of such a function would require either overapproximatic stuff and 
> > doing a looot of extra handling, or becoming flow sensitive... and we'd 
> > still need to understand all the va_ standard functions' semantics either 
> > way.
> 
> Well, that's what I'm wondering, really. The arguments are certainly easy to 
> swap because the type system can't help the user to identify swaps without 
> further information (like format specifier strings). However, the checking 
> code would be... highly unpleasant, I suspect. My intuition is to say that we 
> don't support functions without prototypes at all (we just silently ignore 
> them) and that we only check the typed parameters in a variadic function 
> declaration (e.g., we'll diagnose `void foo(int i, int j, ...);` because of 
> the sequential `int` parameters, but we won't diagnose `void foo(int i, 
> ...);` even if call sites look like `foo(1, 2);`). WDYT?
It is definitely highly unpleasant, at one point I remember just glancing into 
the logic behind `printf()` related warnings in Sema and it was... odd, to say 
the least.

That is how the checker works right now. It will not diagnose `int f() {}` 
because from the looks of the function definition, it is a 0-parameter 
function. Same with the variadic `...`, it is a single parameter function 
(which has a rather //weird// parameter type).

But yes, I think I'll make this explicit in the documentation (and FWIW, the 
research paper, too). 

> However, the checking code would be... highly unpleasant, I suspect.

Incredibly, because this check purposefully targets the function definition 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:177
+   const std::size_t StartIndex) {
+  const std::size_t NumParams = FD->getNumParams();
+  assert(StartIndex < NumParams && "out of bounds for start");

whisperity wrote:
> aaron.ballman wrote:
> > Some interesting test cases to consider: varargs functions and K C 
> > functions
> > K C functions
> 
> Call me too young, but I had to look up what a "K C function" is, and I am 
> absolutely baffled how this unholy construct is still supported! **But:** 
> thanks to Clang supporting it properly in the AST, the checker works out of 
> the box!
> 
> Given
> 
> ```
> int foo(a, b)
>   int a;
>   int b;
> {
>   return a + b;
> }
> ```
> 
> We get the following output:
> 
> ```
> /tmp/knr.c:2:3: warning: 2 adjacent parameters of 'foo' of similar type 
> ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
>   int a;
>   ^
> /tmp/knr.c:2:7: note: the first parameter in the range is 'a'
>   int a;
>   ^
> /tmp/knr.c:3:7: note: the last parameter in the range is 'b'
>   int b;
>   ^
> ```
> 
> (even the locations are consistent!)
> 
> Should I add a test case for this? We could use a specifically C test case 
> either way eventually...
> 
> -
> 
> > varargs functions
> 
> This is a bit of terminology, but something tells me you meant the //variadic 
> function// here, right? As opposed to type parameter packs.
> 
> Given
> 
> ```
> int sum(int ints...) {
>   return 0;
> }
> ```
> 
> the AST looks something like this:
> 
> ```
> `-FunctionDecl 0x56372e29e258  line:1:5 sum 'int 
> (int, ...)'
>   |-ParmVarDecl 0x56372e29e188  col:13 ints 'int'
> ```
> 
> Should we diagnose this? And if so, how? The variadic part is not represented 
> (at least not at first glance?) in the AST. Understanding the insides of such 
> a function would require either overapproximatic stuff and doing a looot of 
> extra handling, or becoming flow sensitive... and we'd still need to 
> understand all the `va_` standard functions' semantics either way.
> Call me too young, but I had to look up what a "K C function" is, and I am 
> absolutely baffled how this unholy construct is still supported!

Ah, to be innocent again, how I miss those days. :-D

> Should I add a test case for this? We could use a specifically C test case 
> either way eventually...

I think it'd be a useful case, but the one I was specifically more concerned 
with is:
```
// N.B.: this is C-specific and does not apply to C++.
void f();

int main(void) {
  f(1, 2, 3.4, "this is why we can't have nice things");
}
```
where the function has no prototype and so is treated as a varargs call.

> This is a bit of terminology, but something tells me you meant the variadic 
> function here, right? As opposed to type parameter packs.

Yes, sorry for being unclear, I am talking about variadic functions.

> Should we diagnose this? And if so, how? The variadic part is not represented 
> (at least not at first glance?) in the AST. Understanding the insides of such 
> a function would require either overapproximatic stuff and doing a looot of 
> extra handling, or becoming flow sensitive... and we'd still need to 
> understand all the va_ standard functions' semantics either way.

Well, that's what I'm wondering, really. The arguments are certainly easy to 
swap because the type system can't help the user to identify swaps without 
further information (like format specifier strings). However, the checking code 
would be... highly unpleasant, I suspect. My intuition is to say that we don't 
support functions without prototypes at all (we just silently ignore them) and 
that we only check the typed parameters in a variadic function declaration 
(e.g., we'll diagnose `void foo(int i, int j, ...);` because of the sequential 
`int` parameters, but we won't diagnose `void foo(int i, ...);` even if call 
sites look like `foo(1, 2);`). WDYT?


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 'bugprone-easily-swappable-parameters' check

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:337
+  if (Name.empty())
+Name = "";
+  return Name;

I think that this will become unreachable after D84658 and D85033. I may be 
missing an argument, but in general I would argue that `getNameForDiagnostic` 
should never return an empty name for unnamed entities since this is not useful 
in diagnostics.


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 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity marked 6 inline comments as done.
whisperity added inline comments.
Herald added a subscriber: nullptr.cpp.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:177
+   const std::size_t StartIndex) {
+  const std::size_t NumParams = FD->getNumParams();
+  assert(StartIndex < NumParams && "out of bounds for start");

aaron.ballman wrote:
> Some interesting test cases to consider: varargs functions and K C functions
> K C functions

Call me too young, but I had to look up what a "K C function" is, and I am 
absolutely baffled how this unholy construct is still supported! **But:** 
thanks to Clang supporting it properly in the AST, the checker works out of the 
box!

Given

```
int foo(a, b)
  int a;
  int b;
{
  return a + b;
}
```

We get the following output:

```
/tmp/knr.c:2:3: warning: 2 adjacent parameters of 'foo' of similar type ('int') 
are easily swapped by mistake [bugprone-easily-swappable-parameters]
  int a;
  ^
/tmp/knr.c:2:7: note: the first parameter in the range is 'a'
  int a;
  ^
/tmp/knr.c:3:7: note: the last parameter in the range is 'b'
  int b;
  ^
```

(even the locations are consistent!)

Should I add a test case for this? We could use a specifically C test case 
either way eventually...

-

> varargs functions

This is a bit of terminology, but something tells me you meant the //variadic 
function// here, right? As opposed to type parameter packs.

Given

```
int sum(int ints...) {
  return 0;
}
```

the AST looks something like this:

```
`-FunctionDecl 0x56372e29e258  line:1:5 sum 'int 
(int, ...)'
  |-ParmVarDecl 0x56372e29e188  col:13 ints 'int'
```

Should we diagnose this? And if so, how? The variadic part is not represented 
(at least not at first glance?) in the AST. Understanding the insides of such a 
function would require either overapproximatic stuff and doing a looot of extra 
handling, or becoming flow sensitive... and we'd still need to understand all 
the `va_` standard functions' semantics either way.


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 'bugprone-easily-swappable-parameters' check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:10
+
+void declaration(int Param, int Other); // NO-WARN: No chance to change this 
function.
+

whisperity wrote:
> aaron.ballman wrote:
> > whisperity wrote:
> > > aaron.ballman wrote:
> > > > whisperity wrote:
> > > > > aaron.ballman wrote:
> > > > > > whisperity wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > whisperity wrote:
> > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > I think this is a case where we could warn when the 
> > > > > > > > > > declaration is outside of a system header (perhaps as an 
> > > > > > > > > > option).
> > > > > > > > > > 
> > > > > > > > > > Thinking about it a bit more, declarations and definitions 
> > > > > > > > > > provide a novel way to get another kind of swap:
> > > > > > > > > > ```
> > > > > > > > > > void func(int x, int y);
> > > > > > > > > > void func(int y, int x) {} // Oops, the swap was probably 
> > > > > > > > > > not intentional
> > > > > > > > > > ```
> > > > > > > > > > which may or may not be interesting for a check (or its 
> > > > > > > > > > heuristics).
> > > > > > > > > I gave this some thought. It is a very good idea, but I 
> > > > > > > > > believe not for //this// check, but D20689. What do you think 
> > > > > > > > > of that? Maybe simply saying "call site v. function node that 
> > > > > > > > > was called" could be extended with a completely analogous, 
> > > > > > > > > string distance function based "function definition node v. 
> > > > > > > > > redecl chain" case.
> > > > > > > > Hmmm, my impression is that this check is fundamentally about 
> > > > > > > > *parameter* ordering whereas D20689 is about *argument* 
> > > > > > > > ordering. Given that the example is about the ordering of 
> > > > > > > > parameters and doesn't have anything to do with call sites, I 
> > > > > > > > kind of think the functionality would be better in a 
> > > > > > > > parameter-oriented check.
> > > > > > > > 
> > > > > > > > That said, it doesn't feel entirely natural to add it to this 
> > > > > > > > check because this check is about parameters that are easily 
> > > > > > > > swappable *at call sites*, and this situation is not exactly 
> > > > > > > > that. However, it could probably be argued that it is 
> > > > > > > > appropriate to add it to this check given that swapped 
> > > > > > > > parameters between the declaration and the definition are 
> > > > > > > > likely to result in swapped arguments at call sites.
> > > > > > > > 
> > > > > > > > Don't feel obligated to add this functionality to this check 
> > > > > > > > (or invent a new check), though.
> > > > > > > It just seems incredibly easier to put it into the other check, 
> > > > > > > as that deals with names. But yes, then it would be a bit weird 
> > > > > > > for the "suspicious call argument" check to say something about 
> > > > > > > the definition... This check (and the relevant Core Guideline) 
> > > > > > > was originally meant to consider **only** types (this is 
> > > > > > > something I'll have to emphasise more in the research paper 
> > > > > > > too!). Everything that considers //names// is only for making the 
> > > > > > > check less noisy and make the actually emitted results more 
> > > > > > > useful. However, I feel we should //specifically// not rely on 
> > > > > > > the names and the logic too much.
> > > > > > > 
> > > > > > > But(!) your suggestion is otherwise a good idea. I am not sure if 
> > > > > > > the previous research (with regards to names) consider the whole 
> > > > > > > "forward declaration" situation, even where they did analysis for 
> > > > > > > C projects, not just Java.
> > > > > > > However, I feel we should specifically not rely on the names and 
> > > > > > > the logic too much.
> > > > > > 
> > > > > > I agree, to a point. I don't think the basic check logic should be 
> > > > > > name-sensitive, but I do think we need to rely on names to tweak 
> > > > > > the true/false positive/negative ratios. I think most of the time 
> > > > > > when we're relying on names should wind up being configuration 
> > > > > > options so that users can tune the algorithm to their needs.
> > > > > > 
> > > > > > We could put the logic into the suspicious call argument check with 
> > > > > > roughly the same logic -- the call site looks like it has 
> > > > > > potentially swapped arguments because the function redeclaration 
> > > > > > chain (which includes the function definition, as that's also a 
> > > > > > declaration) has inconsistent parameter names. So long as the 
> > > > > > diagnostic appears on the call site, and then refers back to the 
> > > > > > inconsistent declarations, it could probably work. However, I think 
> > > > > > it'd be a bit strange to diagnose the call site because the user of 
> > > > > > the API didn't really do anything wrong 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:10
+
+void declaration(int Param, int Other); // NO-WARN: No chance to change this 
function.
+

aaron.ballman wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > whisperity wrote:
> > > > aaron.ballman wrote:
> > > > > whisperity wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > whisperity wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > I think this is a case where we could warn when the 
> > > > > > > > > declaration is outside of a system header (perhaps as an 
> > > > > > > > > option).
> > > > > > > > > 
> > > > > > > > > Thinking about it a bit more, declarations and definitions 
> > > > > > > > > provide a novel way to get another kind of swap:
> > > > > > > > > ```
> > > > > > > > > void func(int x, int y);
> > > > > > > > > void func(int y, int x) {} // Oops, the swap was probably not 
> > > > > > > > > intentional
> > > > > > > > > ```
> > > > > > > > > which may or may not be interesting for a check (or its 
> > > > > > > > > heuristics).
> > > > > > > > I gave this some thought. It is a very good idea, but I believe 
> > > > > > > > not for //this// check, but D20689. What do you think of that? 
> > > > > > > > Maybe simply saying "call site v. function node that was 
> > > > > > > > called" could be extended with a completely analogous, string 
> > > > > > > > distance function based "function definition node v. redecl 
> > > > > > > > chain" case.
> > > > > > > Hmmm, my impression is that this check is fundamentally about 
> > > > > > > *parameter* ordering whereas D20689 is about *argument* ordering. 
> > > > > > > Given that the example is about the ordering of parameters and 
> > > > > > > doesn't have anything to do with call sites, I kind of think the 
> > > > > > > functionality would be better in a parameter-oriented check.
> > > > > > > 
> > > > > > > That said, it doesn't feel entirely natural to add it to this 
> > > > > > > check because this check is about parameters that are easily 
> > > > > > > swappable *at call sites*, and this situation is not exactly 
> > > > > > > that. However, it could probably be argued that it is appropriate 
> > > > > > > to add it to this check given that swapped parameters between the 
> > > > > > > declaration and the definition are likely to result in swapped 
> > > > > > > arguments at call sites.
> > > > > > > 
> > > > > > > Don't feel obligated to add this functionality to this check (or 
> > > > > > > invent a new check), though.
> > > > > > It just seems incredibly easier to put it into the other check, as 
> > > > > > that deals with names. But yes, then it would be a bit weird for 
> > > > > > the "suspicious call argument" check to say something about the 
> > > > > > definition... This check (and the relevant Core Guideline) was 
> > > > > > originally meant to consider **only** types (this is something I'll 
> > > > > > have to emphasise more in the research paper too!). Everything that 
> > > > > > considers //names// is only for making the check less noisy and 
> > > > > > make the actually emitted results more useful. However, I feel we 
> > > > > > should //specifically// not rely on the names and the logic too 
> > > > > > much.
> > > > > > 
> > > > > > But(!) your suggestion is otherwise a good idea. I am not sure if 
> > > > > > the previous research (with regards to names) consider the whole 
> > > > > > "forward declaration" situation, even where they did analysis for C 
> > > > > > projects, not just Java.
> > > > > > However, I feel we should specifically not rely on the names and 
> > > > > > the logic too much.
> > > > > 
> > > > > I agree, to a point. I don't think the basic check logic should be 
> > > > > name-sensitive, but I do think we need to rely on names to tweak the 
> > > > > true/false positive/negative ratios. I think most of the time when 
> > > > > we're relying on names should wind up being configuration options so 
> > > > > that users can tune the algorithm to their needs.
> > > > > 
> > > > > We could put the logic into the suspicious call argument check with 
> > > > > roughly the same logic -- the call site looks like it has potentially 
> > > > > swapped arguments because the function redeclaration chain (which 
> > > > > includes the function definition, as that's also a declaration) has 
> > > > > inconsistent parameter names. So long as the diagnostic appears on 
> > > > > the call site, and then refers back to the inconsistent declarations, 
> > > > > it could probably work. However, I think it'd be a bit strange to 
> > > > > diagnose the call site because the user of the API didn't really do 
> > > > > anything wrong (and they may not even be able to change the 
> > > > > declaration or the definition where the problem really lives).
> > > > But wait... suppose there is a project A, which sees the header 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:10
+
+void declaration(int Param, int Other); // NO-WARN: No chance to change this 
function.
+

whisperity wrote:
> aaron.ballman wrote:
> > whisperity wrote:
> > > aaron.ballman wrote:
> > > > whisperity wrote:
> > > > > aaron.ballman wrote:
> > > > > > whisperity wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > I think this is a case where we could warn when the declaration 
> > > > > > > > is outside of a system header (perhaps as an option).
> > > > > > > > 
> > > > > > > > Thinking about it a bit more, declarations and definitions 
> > > > > > > > provide a novel way to get another kind of swap:
> > > > > > > > ```
> > > > > > > > void func(int x, int y);
> > > > > > > > void func(int y, int x) {} // Oops, the swap was probably not 
> > > > > > > > intentional
> > > > > > > > ```
> > > > > > > > which may or may not be interesting for a check (or its 
> > > > > > > > heuristics).
> > > > > > > I gave this some thought. It is a very good idea, but I believe 
> > > > > > > not for //this// check, but D20689. What do you think of that? 
> > > > > > > Maybe simply saying "call site v. function node that was called" 
> > > > > > > could be extended with a completely analogous, string distance 
> > > > > > > function based "function definition node v. redecl chain" case.
> > > > > > Hmmm, my impression is that this check is fundamentally about 
> > > > > > *parameter* ordering whereas D20689 is about *argument* ordering. 
> > > > > > Given that the example is about the ordering of parameters and 
> > > > > > doesn't have anything to do with call sites, I kind of think the 
> > > > > > functionality would be better in a parameter-oriented check.
> > > > > > 
> > > > > > That said, it doesn't feel entirely natural to add it to this check 
> > > > > > because this check is about parameters that are easily swappable 
> > > > > > *at call sites*, and this situation is not exactly that. However, 
> > > > > > it could probably be argued that it is appropriate to add it to 
> > > > > > this check given that swapped parameters between the declaration 
> > > > > > and the definition are likely to result in swapped arguments at 
> > > > > > call sites.
> > > > > > 
> > > > > > Don't feel obligated to add this functionality to this check (or 
> > > > > > invent a new check), though.
> > > > > It just seems incredibly easier to put it into the other check, as 
> > > > > that deals with names. But yes, then it would be a bit weird for the 
> > > > > "suspicious call argument" check to say something about the 
> > > > > definition... This check (and the relevant Core Guideline) was 
> > > > > originally meant to consider **only** types (this is something I'll 
> > > > > have to emphasise more in the research paper too!). Everything that 
> > > > > considers //names// is only for making the check less noisy and make 
> > > > > the actually emitted results more useful. However, I feel we should 
> > > > > //specifically// not rely on the names and the logic too much.
> > > > > 
> > > > > But(!) your suggestion is otherwise a good idea. I am not sure if the 
> > > > > previous research (with regards to names) consider the whole "forward 
> > > > > declaration" situation, even where they did analysis for C projects, 
> > > > > not just Java.
> > > > > However, I feel we should specifically not rely on the names and the 
> > > > > logic too much.
> > > > 
> > > > I agree, to a point. I don't think the basic check logic should be 
> > > > name-sensitive, but I do think we need to rely on names to tweak the 
> > > > true/false positive/negative ratios. I think most of the time when 
> > > > we're relying on names should wind up being configuration options so 
> > > > that users can tune the algorithm to their needs.
> > > > 
> > > > We could put the logic into the suspicious call argument check with 
> > > > roughly the same logic -- the call site looks like it has potentially 
> > > > swapped arguments because the function redeclaration chain (which 
> > > > includes the function definition, as that's also a declaration) has 
> > > > inconsistent parameter names. So long as the diagnostic appears on the 
> > > > call site, and then refers back to the inconsistent declarations, it 
> > > > could probably work. However, I think it'd be a bit strange to diagnose 
> > > > the call site because the user of the API didn't really do anything 
> > > > wrong (and they may not even be able to change the declaration or the 
> > > > definition where the problem really lives).
> > > But wait... suppose there is a project A, which sees the header for 
> > > `f(int x, int y)` and has a call `f(y, x)`. That will be diagnosed. But 
> > > if project A is only a client of `f`, it should be highly unlikely that 
> > > project A has the //definition// of `f` in their 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:10
+
+void declaration(int Param, int Other); // NO-WARN: No chance to change this 
function.
+

aaron.ballman wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > whisperity wrote:
> > > > aaron.ballman wrote:
> > > > > whisperity wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > I think this is a case where we could warn when the declaration 
> > > > > > > is outside of a system header (perhaps as an option).
> > > > > > > 
> > > > > > > Thinking about it a bit more, declarations and definitions 
> > > > > > > provide a novel way to get another kind of swap:
> > > > > > > ```
> > > > > > > void func(int x, int y);
> > > > > > > void func(int y, int x) {} // Oops, the swap was probably not 
> > > > > > > intentional
> > > > > > > ```
> > > > > > > which may or may not be interesting for a check (or its 
> > > > > > > heuristics).
> > > > > > I gave this some thought. It is a very good idea, but I believe not 
> > > > > > for //this// check, but D20689. What do you think of that? Maybe 
> > > > > > simply saying "call site v. function node that was called" could be 
> > > > > > extended with a completely analogous, string distance function 
> > > > > > based "function definition node v. redecl chain" case.
> > > > > Hmmm, my impression is that this check is fundamentally about 
> > > > > *parameter* ordering whereas D20689 is about *argument* ordering. 
> > > > > Given that the example is about the ordering of parameters and 
> > > > > doesn't have anything to do with call sites, I kind of think the 
> > > > > functionality would be better in a parameter-oriented check.
> > > > > 
> > > > > That said, it doesn't feel entirely natural to add it to this check 
> > > > > because this check is about parameters that are easily swappable *at 
> > > > > call sites*, and this situation is not exactly that. However, it 
> > > > > could probably be argued that it is appropriate to add it to this 
> > > > > check given that swapped parameters between the declaration and the 
> > > > > definition are likely to result in swapped arguments at call sites.
> > > > > 
> > > > > Don't feel obligated to add this functionality to this check (or 
> > > > > invent a new check), though.
> > > > It just seems incredibly easier to put it into the other check, as that 
> > > > deals with names. But yes, then it would be a bit weird for the 
> > > > "suspicious call argument" check to say something about the 
> > > > definition... This check (and the relevant Core Guideline) was 
> > > > originally meant to consider **only** types (this is something I'll 
> > > > have to emphasise more in the research paper too!). Everything that 
> > > > considers //names// is only for making the check less noisy and make 
> > > > the actually emitted results more useful. However, I feel we should 
> > > > //specifically// not rely on the names and the logic too much.
> > > > 
> > > > But(!) your suggestion is otherwise a good idea. I am not sure if the 
> > > > previous research (with regards to names) consider the whole "forward 
> > > > declaration" situation, even where they did analysis for C projects, 
> > > > not just Java.
> > > > However, I feel we should specifically not rely on the names and the 
> > > > logic too much.
> > > 
> > > I agree, to a point. I don't think the basic check logic should be 
> > > name-sensitive, but I do think we need to rely on names to tweak the 
> > > true/false positive/negative ratios. I think most of the time when we're 
> > > relying on names should wind up being configuration options so that users 
> > > can tune the algorithm to their needs.
> > > 
> > > We could put the logic into the suspicious call argument check with 
> > > roughly the same logic -- the call site looks like it has potentially 
> > > swapped arguments because the function redeclaration chain (which 
> > > includes the function definition, as that's also a declaration) has 
> > > inconsistent parameter names. So long as the diagnostic appears on the 
> > > call site, and then refers back to the inconsistent declarations, it 
> > > could probably work. However, I think it'd be a bit strange to diagnose 
> > > the call site because the user of the API didn't really do anything wrong 
> > > (and they may not even be able to change the declaration or the 
> > > definition where the problem really lives).
> > But wait... suppose there is a project A, which sees the header for `f(int 
> > x, int y)` and has a call `f(y, x)`. That will be diagnosed. But if project 
> > A is only a client of `f`, it should be highly unlikely that project A has 
> > the //definition// of `f` in their tree, right? So people of A will not get 
> > the warning for that. Only what is part of the compilation process will be 
> > part of the analysis process.
> > 
> > Similarly to how 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:10
+
+void declaration(int Param, int Other); // NO-WARN: No chance to change this 
function.
+

whisperity wrote:
> aaron.ballman wrote:
> > whisperity wrote:
> > > aaron.ballman wrote:
> > > > whisperity wrote:
> > > > > aaron.ballman wrote:
> > > > > > I think this is a case where we could warn when the declaration is 
> > > > > > outside of a system header (perhaps as an option).
> > > > > > 
> > > > > > Thinking about it a bit more, declarations and definitions provide 
> > > > > > a novel way to get another kind of swap:
> > > > > > ```
> > > > > > void func(int x, int y);
> > > > > > void func(int y, int x) {} // Oops, the swap was probably not 
> > > > > > intentional
> > > > > > ```
> > > > > > which may or may not be interesting for a check (or its heuristics).
> > > > > I gave this some thought. It is a very good idea, but I believe not 
> > > > > for //this// check, but D20689. What do you think of that? Maybe 
> > > > > simply saying "call site v. function node that was called" could be 
> > > > > extended with a completely analogous, string distance function based 
> > > > > "function definition node v. redecl chain" case.
> > > > Hmmm, my impression is that this check is fundamentally about 
> > > > *parameter* ordering whereas D20689 is about *argument* ordering. Given 
> > > > that the example is about the ordering of parameters and doesn't have 
> > > > anything to do with call sites, I kind of think the functionality would 
> > > > be better in a parameter-oriented check.
> > > > 
> > > > That said, it doesn't feel entirely natural to add it to this check 
> > > > because this check is about parameters that are easily swappable *at 
> > > > call sites*, and this situation is not exactly that. However, it could 
> > > > probably be argued that it is appropriate to add it to this check given 
> > > > that swapped parameters between the declaration and the definition are 
> > > > likely to result in swapped arguments at call sites.
> > > > 
> > > > Don't feel obligated to add this functionality to this check (or invent 
> > > > a new check), though.
> > > It just seems incredibly easier to put it into the other check, as that 
> > > deals with names. But yes, then it would be a bit weird for the 
> > > "suspicious call argument" check to say something about the definition... 
> > > This check (and the relevant Core Guideline) was originally meant to 
> > > consider **only** types (this is something I'll have to emphasise more in 
> > > the research paper too!). Everything that considers //names// is only for 
> > > making the check less noisy and make the actually emitted results more 
> > > useful. However, I feel we should //specifically// not rely on the names 
> > > and the logic too much.
> > > 
> > > But(!) your suggestion is otherwise a good idea. I am not sure if the 
> > > previous research (with regards to names) consider the whole "forward 
> > > declaration" situation, even where they did analysis for C projects, not 
> > > just Java.
> > > However, I feel we should specifically not rely on the names and the 
> > > logic too much.
> > 
> > I agree, to a point. I don't think the basic check logic should be 
> > name-sensitive, but I do think we need to rely on names to tweak the 
> > true/false positive/negative ratios. I think most of the time when we're 
> > relying on names should wind up being configuration options so that users 
> > can tune the algorithm to their needs.
> > 
> > We could put the logic into the suspicious call argument check with roughly 
> > the same logic -- the call site looks like it has potentially swapped 
> > arguments because the function redeclaration chain (which includes the 
> > function definition, as that's also a declaration) has inconsistent 
> > parameter names. So long as the diagnostic appears on the call site, and 
> > then refers back to the inconsistent declarations, it could probably work. 
> > However, I think it'd be a bit strange to diagnose the call site because 
> > the user of the API didn't really do anything wrong (and they may not even 
> > be able to change the declaration or the definition where the problem 
> > really lives).
> But wait... suppose there is a project A, which sees the header for `f(int x, 
> int y)` and has a call `f(y, x)`. That will be diagnosed. But if project A is 
> only a client of `f`, it should be highly unlikely that project A has the 
> //definition// of `f` in their tree, right? So people of A will not get the 
> warning for that. Only what is part of the compilation process will be part 
> of the analysis process.
> 
> Similarly to how this check matches **only** function definitions, and never 
> a prototype. The latter one would have bazillion of warnings from every 
> header ever, with the user not having a chance to fix 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:402-403
+
+// Unfortunately, undersquiggly highlights are for some reason chewed up
+// and not respected by diagnostics from Clang-Tidy. :(
+diag(First->getLocation(), "the first parameter in the range is '%0'",

aaron.ballman wrote:
> whisperity wrote:
> > whisperity wrote:
> > > aaron.ballman wrote:
> > > > whisperity wrote:
> > > > > aaron.ballman wrote:
> > > > > > Can you post a screenshot of what you mean?
> > > > > Given
> > > > > 
> > > > > ```
> > > > > Diag << SourceRange{First->getSourceRange().getBegin(), 
> > > > > Last->getSourceRange().getEnd()};
> > > > > ```
> > > > > 
> > > > > The diagnostic is still produced with only a `^` at the original 
> > > > > `diag(Location)`, ignoring the fed `SourceRange`:
> > > > > 
> > > > > ```
> > > > > /home/whisperity/LLVM/Build/../llvm-project/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:21:18:
> > > > >  warning: 3 adjacent parameters of 'redeclChain' of similar type 
> > > > > ('int') are easily swapped by mistake 
> > > > > [bugprone-easily-swappable-parameters]
> > > > > void redeclChain(int I, int J, int K) {}
> > > > >  ^
> > > > > ```
> > > > > 
> > > > > Instead of putting all the squigglies in as the range-location 
> > > > > highlight, like how `Sema` can diagnose:
> > > > > 
> > > > > ```
> > > > > /home/whisperity/LLVM/Build/../llvm-project/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:21:18:
> > > > >  warning: 3 adjacent parameters of 'redeclChain' of similar type 
> > > > > ('int') are easily swapped by mistake 
> > > > > [bugprone-easily-swappable-parameters]
> > > > > void redeclChain(int I, int J, int K) {}
> > > > >  ^~~
> > > > > ```
> > > > > 
> > > > > I would not want to put additional note tags in an otherwise already 
> > > > > verbose output.
> > > > > 
> > > > > Back in 2019 I was investigating this issue specifically for another 
> > > > > checker I was working on, but hit the wall... Somewhere deep inside 
> > > > > where Tidy diagnostic stuff is translated and consumed to Clang 
> > > > > stuff, it goes away. It shouldn't, because there are statements that 
> > > > > seemingly copy the `Diag.Ranges` array over, but it still does not 
> > > > > come out.
> > > > > 
> > > > > ... EDIT: I found [[ 
> > > > > http://lists.llvm.org/pipermail/cfe-dev/2019-October/063676.html | 
> > > > > the relevant mailing list post ]].
> > > > > ... EDIT: I found the relevant mailing list post.
> > > > 
> > > > Oh wow, I had no idea! That's a rather unfortunate bug. :-(
> > > Speaking of bugs, shall I put it up on Bugzilla, or see if anyone 
> > > reported already? Would require me digging into that "where are the data 
> > > structures copied" kind of thing to figure it out again.
> > Nevermind, this is worth [[ http://bugzilla.llvm.org/show_bug.cgi?id=49000 
> > | a bug report ]] just so we can track it in the proper location.
> If you don't mind doing that effort, it'd be appreciated! Even if you don't 
> root cause the issue, having the report (or pinging an existing one) is still 
> useful.
And I managed to post a broken link originally... Here's the right one: 
https://bugs.llvm.org/show_bug.cgi?id=49000


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 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:10
+
+void declaration(int Param, int Other); // NO-WARN: No chance to change this 
function.
+

aaron.ballman wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > whisperity wrote:
> > > > aaron.ballman wrote:
> > > > > I think this is a case where we could warn when the declaration is 
> > > > > outside of a system header (perhaps as an option).
> > > > > 
> > > > > Thinking about it a bit more, declarations and definitions provide a 
> > > > > novel way to get another kind of swap:
> > > > > ```
> > > > > void func(int x, int y);
> > > > > void func(int y, int x) {} // Oops, the swap was probably not 
> > > > > intentional
> > > > > ```
> > > > > which may or may not be interesting for a check (or its heuristics).
> > > > I gave this some thought. It is a very good idea, but I believe not for 
> > > > //this// check, but D20689. What do you think of that? Maybe simply 
> > > > saying "call site v. function node that was called" could be extended 
> > > > with a completely analogous, string distance function based "function 
> > > > definition node v. redecl chain" case.
> > > Hmmm, my impression is that this check is fundamentally about *parameter* 
> > > ordering whereas D20689 is about *argument* ordering. Given that the 
> > > example is about the ordering of parameters and doesn't have anything to 
> > > do with call sites, I kind of think the functionality would be better in 
> > > a parameter-oriented check.
> > > 
> > > That said, it doesn't feel entirely natural to add it to this check 
> > > because this check is about parameters that are easily swappable *at call 
> > > sites*, and this situation is not exactly that. However, it could 
> > > probably be argued that it is appropriate to add it to this check given 
> > > that swapped parameters between the declaration and the definition are 
> > > likely to result in swapped arguments at call sites.
> > > 
> > > Don't feel obligated to add this functionality to this check (or invent a 
> > > new check), though.
> > It just seems incredibly easier to put it into the other check, as that 
> > deals with names. But yes, then it would be a bit weird for the "suspicious 
> > call argument" check to say something about the definition... This check 
> > (and the relevant Core Guideline) was originally meant to consider **only** 
> > types (this is something I'll have to emphasise more in the research paper 
> > too!). Everything that considers //names// is only for making the check 
> > less noisy and make the actually emitted results more useful. However, I 
> > feel we should //specifically// not rely on the names and the logic too 
> > much.
> > 
> > But(!) your suggestion is otherwise a good idea. I am not sure if the 
> > previous research (with regards to names) consider the whole "forward 
> > declaration" situation, even where they did analysis for C projects, not 
> > just Java.
> > However, I feel we should specifically not rely on the names and the logic 
> > too much.
> 
> I agree, to a point. I don't think the basic check logic should be 
> name-sensitive, but I do think we need to rely on names to tweak the 
> true/false positive/negative ratios. I think most of the time when we're 
> relying on names should wind up being configuration options so that users can 
> tune the algorithm to their needs.
> 
> We could put the logic into the suspicious call argument check with roughly 
> the same logic -- the call site looks like it has potentially swapped 
> arguments because the function redeclaration chain (which includes the 
> function definition, as that's also a declaration) has inconsistent parameter 
> names. So long as the diagnostic appears on the call site, and then refers 
> back to the inconsistent declarations, it could probably work. However, I 
> think it'd be a bit strange to diagnose the call site because the user of the 
> API didn't really do anything wrong (and they may not even be able to change 
> the declaration or the definition where the problem really lives).
But wait... suppose there is a project A, which sees the header for `f(int x, 
int y)` and has a call `f(y, x)`. That will be diagnosed. But if project A is 
only a client of `f`, it should be highly unlikely that project A has the 
//definition// of `f` in their tree, right? So people of A will not get the 
warning for that. Only what is part of the compilation process will be part of 
the analysis process.

Similarly to how this check matches **only** function definitions, and never a 
prototype. The latter one would have bazillion of warnings from every header 
ever, with the user not having a chance to fix it anyways.


Repository:
  rG LLVM Github Monorepo

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

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D69560#2536570 , @steveire wrote:

> I haven't read through all the comments, but the word 'easily' implies 
> 'desirable'. This check seems to be for finding params which are undesirably 
> swappable, right?

The `easily` was to mean that the swap can be done with little effort (i.e. "in 
an easy fashion"?) and by accident. We want to find function signatures where 
that is the case, yes. TL;DR: //"This function will be misused by a careless 
client, please try designing a better interface!"//


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 'bugprone-easily-swappable-parameters' check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:402-403
+
+// Unfortunately, undersquiggly highlights are for some reason chewed up
+// and not respected by diagnostics from Clang-Tidy. :(
+diag(First->getLocation(), "the first parameter in the range is '%0'",

whisperity wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > whisperity wrote:
> > > > aaron.ballman wrote:
> > > > > Can you post a screenshot of what you mean?
> > > > Given
> > > > 
> > > > ```
> > > > Diag << SourceRange{First->getSourceRange().getBegin(), 
> > > > Last->getSourceRange().getEnd()};
> > > > ```
> > > > 
> > > > The diagnostic is still produced with only a `^` at the original 
> > > > `diag(Location)`, ignoring the fed `SourceRange`:
> > > > 
> > > > ```
> > > > /home/whisperity/LLVM/Build/../llvm-project/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:21:18:
> > > >  warning: 3 adjacent parameters of 'redeclChain' of similar type 
> > > > ('int') are easily swapped by mistake 
> > > > [bugprone-easily-swappable-parameters]
> > > > void redeclChain(int I, int J, int K) {}
> > > >  ^
> > > > ```
> > > > 
> > > > Instead of putting all the squigglies in as the range-location 
> > > > highlight, like how `Sema` can diagnose:
> > > > 
> > > > ```
> > > > /home/whisperity/LLVM/Build/../llvm-project/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:21:18:
> > > >  warning: 3 adjacent parameters of 'redeclChain' of similar type 
> > > > ('int') are easily swapped by mistake 
> > > > [bugprone-easily-swappable-parameters]
> > > > void redeclChain(int I, int J, int K) {}
> > > >  ^~~
> > > > ```
> > > > 
> > > > I would not want to put additional note tags in an otherwise already 
> > > > verbose output.
> > > > 
> > > > Back in 2019 I was investigating this issue specifically for another 
> > > > checker I was working on, but hit the wall... Somewhere deep inside 
> > > > where Tidy diagnostic stuff is translated and consumed to Clang stuff, 
> > > > it goes away. It shouldn't, because there are statements that seemingly 
> > > > copy the `Diag.Ranges` array over, but it still does not come out.
> > > > 
> > > > ... EDIT: I found [[ 
> > > > http://lists.llvm.org/pipermail/cfe-dev/2019-October/063676.html | the 
> > > > relevant mailing list post ]].
> > > > ... EDIT: I found the relevant mailing list post.
> > > 
> > > Oh wow, I had no idea! That's a rather unfortunate bug. :-(
> > Speaking of bugs, shall I put it up on Bugzilla, or see if anyone reported 
> > already? Would require me digging into that "where are the data structures 
> > copied" kind of thing to figure it out again.
> Nevermind, this is worth [[ http://bugzilla.llvm.org/show_bug.cgi?id=49000 | 
> a bug report ]] just so we can track it in the proper location.
If you don't mind doing that effort, it'd be appreciated! Even if you don't 
root cause the issue, having the report (or pinging an existing one) is still 
useful.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:10
+
+void declaration(int Param, int Other); // NO-WARN: No chance to change this 
function.
+

whisperity wrote:
> aaron.ballman wrote:
> > whisperity wrote:
> > > aaron.ballman wrote:
> > > > I think this is a case where we could warn when the declaration is 
> > > > outside of a system header (perhaps as an option).
> > > > 
> > > > Thinking about it a bit more, declarations and definitions provide a 
> > > > novel way to get another kind of swap:
> > > > ```
> > > > void func(int x, int y);
> > > > void func(int y, int x) {} // Oops, the swap was probably not 
> > > > intentional
> > > > ```
> > > > which may or may not be interesting for a check (or its heuristics).
> > > I gave this some thought. It is a very good idea, but I believe not for 
> > > //this// check, but D20689. What do you think of that? Maybe simply 
> > > saying "call site v. function node that was called" could be extended 
> > > with a completely analogous, string distance function based "function 
> > > definition node v. redecl chain" case.
> > Hmmm, my impression is that this check is fundamentally about *parameter* 
> > ordering whereas D20689 is about *argument* ordering. Given that the 
> > example is about the ordering of parameters and doesn't have anything to do 
> > with call sites, I kind of think the functionality would be better in a 
> > parameter-oriented check.
> > 
> > That said, it doesn't feel entirely natural to add it to this check because 
> > this check is about parameters that are easily swappable *at call sites*, 
> > and this situation is not exactly that. However, it could probably be 
> > argued that it is appropriate to add it to this check given that swapped 
> 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

I haven't read through all the comments, but the word 'easily' implies 
'desirable'. This check seems to be for finding params which are undesirably 
swappable, right?


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 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity marked 3 inline comments as done.
whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:402-403
+
+// Unfortunately, undersquiggly highlights are for some reason chewed up
+// and not respected by diagnostics from Clang-Tidy. :(
+diag(First->getLocation(), "the first parameter in the range is '%0'",

whisperity wrote:
> aaron.ballman wrote:
> > whisperity wrote:
> > > aaron.ballman wrote:
> > > > Can you post a screenshot of what you mean?
> > > Given
> > > 
> > > ```
> > > Diag << SourceRange{First->getSourceRange().getBegin(), 
> > > Last->getSourceRange().getEnd()};
> > > ```
> > > 
> > > The diagnostic is still produced with only a `^` at the original 
> > > `diag(Location)`, ignoring the fed `SourceRange`:
> > > 
> > > ```
> > > /home/whisperity/LLVM/Build/../llvm-project/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:21:18:
> > >  warning: 3 adjacent parameters of 'redeclChain' of similar type ('int') 
> > > are easily swapped by mistake [bugprone-easily-swappable-parameters]
> > > void redeclChain(int I, int J, int K) {}
> > >  ^
> > > ```
> > > 
> > > Instead of putting all the squigglies in as the range-location highlight, 
> > > like how `Sema` can diagnose:
> > > 
> > > ```
> > > /home/whisperity/LLVM/Build/../llvm-project/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:21:18:
> > >  warning: 3 adjacent parameters of 'redeclChain' of similar type ('int') 
> > > are easily swapped by mistake [bugprone-easily-swappable-parameters]
> > > void redeclChain(int I, int J, int K) {}
> > >  ^~~
> > > ```
> > > 
> > > I would not want to put additional note tags in an otherwise already 
> > > verbose output.
> > > 
> > > Back in 2019 I was investigating this issue specifically for another 
> > > checker I was working on, but hit the wall... Somewhere deep inside where 
> > > Tidy diagnostic stuff is translated and consumed to Clang stuff, it goes 
> > > away. It shouldn't, because there are statements that seemingly copy the 
> > > `Diag.Ranges` array over, but it still does not come out.
> > > 
> > > ... EDIT: I found [[ 
> > > http://lists.llvm.org/pipermail/cfe-dev/2019-October/063676.html | the 
> > > relevant mailing list post ]].
> > > ... EDIT: I found the relevant mailing list post.
> > 
> > Oh wow, I had no idea! That's a rather unfortunate bug. :-(
> Speaking of bugs, shall I put it up on Bugzilla, or see if anyone reported 
> already? Would require me digging into that "where are the data structures 
> copied" kind of thing to figure it out again.
Nevermind, this is worth [[ http://bugzilla.llvm.org/show_bug.cgi?id=49000 | a 
bug report ]] just so we can track it in the proper location.


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 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity marked 6 inline comments as done.
whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:255
+  std::string NodeTypeName =
+  Node->getType().getAsString(Node->getASTContext().getPrintingPolicy());
+  StringRef CaptureName{NodeTypeName};

aaron.ballman wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > Hmm, one downside to using the printing policy to get the node name is 
> > > that there can be alternative spellings for the same type that the user 
> > > might reasonably expect to be applied. e.g., if the user expects to 
> > > ignore `bool` datatypes and the printing policy wants to spell the type 
> > > as `_Bool`, this won't behave as expected.
> > But then the diagnostic is inconsistent with what the compiler is 
> > configured to output as diagnostics, isn't it? Can I stringify through 
> > Clang with some "default" printing policy?
> The situation I'm worried about is something like this in C code:
> ```
> #include 
> 
> void func(bool b, bool c) {}
> ```
> because the `bool` in the function signature will expand to `_Bool` after the 
> preprocessor gets to it (because of the contents of ``). If the 
> user writes `bool` in their configuration file because that's what they've 
> written in their function signatures, will this fail because the printing 
> policy says it should be `_Bool`?
> 
> Along similar lines, I wonder about things like `struct S` vs `S` (in C), 
> `signed char` vs `char`, types within anonymous namespaces (where we print 
> ``), etc. Basically, any time when the printing policy 
> may differ from what the user lexically writes in code.
Meanwhile, I realised we are talking of two entirely distinct things. I will 
look into how this `PrintingPolicy` stuff works... I agree that the ignore 
rules (where the comment was originally posted) should respect the text written 
into the code as-is. The diagnostics printing type names could continue using 
the proper printing policy (to be in line with how Sema diagnoses, AFAIK).



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:402-403
+
+// Unfortunately, undersquiggly highlights are for some reason chewed up
+// and not respected by diagnostics from Clang-Tidy. :(
+diag(First->getLocation(), "the first parameter in the range is '%0'",

aaron.ballman wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > Can you post a screenshot of what you mean?
> > Given
> > 
> > ```
> > Diag << SourceRange{First->getSourceRange().getBegin(), 
> > Last->getSourceRange().getEnd()};
> > ```
> > 
> > The diagnostic is still produced with only a `^` at the original 
> > `diag(Location)`, ignoring the fed `SourceRange`:
> > 
> > ```
> > /home/whisperity/LLVM/Build/../llvm-project/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:21:18:
> >  warning: 3 adjacent parameters of 'redeclChain' of similar type ('int') 
> > are easily swapped by mistake [bugprone-easily-swappable-parameters]
> > void redeclChain(int I, int J, int K) {}
> >  ^
> > ```
> > 
> > Instead of putting all the squigglies in as the range-location highlight, 
> > like how `Sema` can diagnose:
> > 
> > ```
> > /home/whisperity/LLVM/Build/../llvm-project/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:21:18:
> >  warning: 3 adjacent parameters of 'redeclChain' of similar type ('int') 
> > are easily swapped by mistake [bugprone-easily-swappable-parameters]
> > void redeclChain(int I, int J, int K) {}
> >  ^~~
> > ```
> > 
> > I would not want to put additional note tags in an otherwise already 
> > verbose output.
> > 
> > Back in 2019 I was investigating this issue specifically for another 
> > checker I was working on, but hit the wall... Somewhere deep inside where 
> > Tidy diagnostic stuff is translated and consumed to Clang stuff, it goes 
> > away. It shouldn't, because there are statements that seemingly copy the 
> > `Diag.Ranges` array over, but it still does not come out.
> > 
> > ... EDIT: I found [[ 
> > http://lists.llvm.org/pipermail/cfe-dev/2019-October/063676.html | the 
> > relevant mailing list post ]].
> > ... EDIT: I found the relevant mailing list post.
> 
> Oh wow, I had no idea! That's a rather unfortunate bug. :-(
Speaking of bugs, shall I put it up on Bugzilla, or see if anyone reported 
already? Would require me digging into that "where are the data structures 
copied" kind of thing to figure it out again.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h:30
+  void storeOptions(ClangTidyOptions::OptionMap ) override;
+
+  /// The minimum length of an adjacent swappable parameter range required for

aaron.ballman wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:58
+
+namespace {
+

whisperity wrote:
> aaron.ballman wrote:
> > Is there a need for the anonymous namespace? (We typically only use them 
> > when defining a class and try to make them as narrow in scope as possible, 
> > and use `static` for function declarations instead.)
> There will be two major parts, the modelling (and which is extended down the 
> line with new models like implicit conversions) and the filtering (which 
> throws away or breaks up results). Should I cut out the anonymous namespace? 
> How about the inner namespaces, may they stay?
The inner namespaces are totally fine. As for the anon namespace, I personally 
don't have a strong opinion against it, I brought it up mostly as a coding 
style guide nit, but I think removing it is helpful so that it's more obvious 
when a function has internal vs external linkage from its signature.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:67
+/// The language features involved in allowing the mix between two parameters.
+enum MixFlags : unsigned char {
+  MIX_Invalid = 0, //< Sentinel bit pattern. DO NOT USE!

whisperity wrote:
> aaron.ballman wrote:
> > Does using the fixed underlying type buy us something? As best I can tell, 
> > this will introduce a fair amount of implicit promotions to `int` anyway.
> Will it? I thought if I use the proper LLVM bitmask/flag enum thing it will 
> work properly. For now, it is good for the bit flag debug print, because it 
> only prints 8 numbers, not 32.
Ah, perhaps that's the case -- I'm not super familiar with the LLVM 
bitmask/flag macros. That said, if you think there's good utility for it, I'm 
happy to leave it as-is.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:90-91
+
+  void sanitise() {
+assert(Flags != MIX_Invalid && "sanitise() called on invalid bitvec");
+// TODO: There will be statements here in further extensions of the check.

whisperity wrote:
> aaron.ballman wrote:
> > Heh, I don't know if we have any guidance on US English vs British English 
> > spellings. ;-)
> Reflexes, my bad. (Similarly how I put the pointer `*` to the left side, 
> where Stroustrup intended. At least my local pre-commit hooks take care of 
> that.) Will try to remember this.
Heh, no worries, I'm betting if we did some code searches, we'd find plenty of 
British English spellings as well. :-D



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:255
+  std::string NodeTypeName =
+  Node->getType().getAsString(Node->getASTContext().getPrintingPolicy());
+  StringRef CaptureName{NodeTypeName};

whisperity wrote:
> aaron.ballman wrote:
> > Hmm, one downside to using the printing policy to get the node name is that 
> > there can be alternative spellings for the same type that the user might 
> > reasonably expect to be applied. e.g., if the user expects to ignore `bool` 
> > datatypes and the printing policy wants to spell the type as `_Bool`, this 
> > won't behave as expected.
> But then the diagnostic is inconsistent with what the compiler is configured 
> to output as diagnostics, isn't it? Can I stringify through Clang with some 
> "default" printing policy?
The situation I'm worried about is something like this in C code:
```
#include 

void func(bool b, bool c) {}
```
because the `bool` in the function signature will expand to `_Bool` after the 
preprocessor gets to it (because of the contents of ``). If the user 
writes `bool` in their configuration file because that's what they've written 
in their function signatures, will this fail because the printing policy says 
it should be `_Bool`?

Along similar lines, I wonder about things like `struct S` vs `S` (in C), 
`signed char` vs `char`, types within anonymous namespaces (where we print 
``), etc. Basically, any time when the printing policy may 
differ from what the user lexically writes in code.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:302
+ "FowardIt", "bidirit", "BidirIt", "constiterator",
+ "const_iterator", "Const_Iterator", "Constiterator",
+ "ConstIterator"});

whisperity wrote:
> aaron.ballman wrote:
> > `reverse_iterator` and `reverse_const_iterator` too?
> > 
> > How about ranges?
> > How about ranges?
> 
> I would like to say that having an `f(RangeTy, RangeTy)` is //exactly// as 
> problematic (especially if both are non-const) as having `f(int, int)` or 
> `f(void*, void*)`, and should be taken care of the same way (relatedness 
> heuristic, name heuristic).
> The idea behind ignoring iterator-ly named things was that 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:284
+/// The default value for the MinimumLength check option.
+static constexpr unsigned DefaultMinimumLength = 2;
+

aaron.ballman wrote:
> It might be good to move this to a more prominent location since it's a 
> default value.
Moved it to the top.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:302
+ "FowardIt", "bidirit", "BidirIt", "constiterator",
+ "const_iterator", "Const_Iterator", "Constiterator",
+ "ConstIterator"});

aaron.ballman wrote:
> `reverse_iterator` and `reverse_const_iterator` too?
> 
> How about ranges?
> How about ranges?

I would like to say that having an `f(RangeTy, RangeTy)` is //exactly// as 
problematic (especially if both are non-const) as having `f(int, int)` or 
`f(void*, void*)`, and should be taken care of the same way (relatedness 
heuristic, name heuristic).
The idea behind ignoring iterator-ly named things was that //"Iterators more 
often than not comes in pairs and you can't(*) do anything about it"//.

(*): Use ranges. Similarly how `draw(int x, int y)` is `draw(Point2D)` if we 
are enhancing type safety.


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 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity updated this revision to Diff 319840.
whisperity marked 9 inline comments as done.
whisperity added a comment.

- NFC Code style and organisation fixes


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/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 3}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""} \
+// RUN:  ]}' --
+
+int add(int Left, int Right) { return Left + Right; } // NO-WARN: Only 2 parameters.
+
+int magic(int Left, int Right, int X, int Y) { return 0; }
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 4 adjacent parameters of 'magic' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:15: note: the first parameter in the range is 'Left'
+// CHECK-MESSAGES: :[[@LINE-3]]:43: note: the last parameter in the range is 'Y'
+
+void multipleDistinctTypes(int I, int J, int K,
+   long L, long M,
+   double D, double E, double F) {}
+// CHECK-MESSAGES: :[[@LINE-3]]:28: warning: 3 adjacent parameters of 'multipleDistinctTypes' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-4]]:32: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-5]]:46: note: the last parameter in the range is 'K'
+// NO-WARN: The [long, long] range is length of 2.
+// CHECK-MESSAGES: :[[@LINE-5]]:28: warning: 3 adjacent parameters of 'multipleDistinctTypes' of similar type ('double')
+// CHECK-MESSAGES: :[[@LINE-6]]:35: note: the first parameter in the range is 'D'
+// CHECK-MESSAGES: :[[@LINE-7]]:55: note: the last parameter in the range is 'F'
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
@@ -0,0 +1,141 @@
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""} \
+// RUN:  ]}' --
+
+#define assert(X)
+
+void declaration(int Param, int Other); // NO-WARN: No chance to change this function.
+
+struct S {};
+
+S *allocate() { return nullptr; }   // NO-WARN: 0 parameters.
+void allocate(S **Out) {}   // NO-WARN: 1 parameter.
+bool operator<(const S , const S ) { return true; } // NO-WARN: Operator.
+
+void redeclChain(int, int, int);
+void redeclChain(int I, int, int);
+void redeclChain(int, int J, int);
+void redeclChain(int I, int J, int K) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 3 adjacent parameters of 'redeclChain' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:22: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:36: note: the last parameter in the range is 'K'
+
+void copyMany(S *Src, S *Dst, unsigned Num) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 2 adjacent parameters of 'copyMany' of similar type ('S *')
+// CHECK-MESSAGES: :[[@LINE-2]]:18: note: the first parameter in the range is 'Src'
+// CHECK-MESSAGES: :[[@LINE-3]]:26: note: the last parameter in the range is 'Dst'
+
+template 
+bool binaryPredicate(T L, U R) { return false; } // NO-WARN: Distinct types in template.
+
+template 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:10
+
+void declaration(int Param, int Other); // NO-WARN: No chance to change this 
function.
+

aaron.ballman wrote:
> I think this is a case where we could warn when the declaration is outside of 
> a system header (perhaps as an option).
> 
> Thinking about it a bit more, declarations and definitions provide a novel 
> way to get another kind of swap:
> ```
> void func(int x, int y);
> void func(int y, int x) {} // Oops, the swap was probably not intentional
> ```
> which may or may not be interesting for a check (or its heuristics).
I gave this some thought. It is a very good idea, but I believe not for 
//this// check, but D20689. What do you think of that? Maybe simply saying 
"call site v. function node that was called" could be extended with a 
completely analogous, string distance function based "function definition node 
v. redecl chain" case.


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 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:58
+
+namespace {
+

aaron.ballman wrote:
> Is there a need for the anonymous namespace? (We typically only use them when 
> defining a class and try to make them as narrow in scope as possible, and use 
> `static` for function declarations instead.)
There will be two major parts, the modelling (and which is extended down the 
line with new models like implicit conversions) and the filtering (which throws 
away or breaks up results). Should I cut out the anonymous namespace? How about 
the inner namespaces, may they stay?



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:67
+/// The language features involved in allowing the mix between two parameters.
+enum MixFlags : unsigned char {
+  MIX_Invalid = 0, //< Sentinel bit pattern. DO NOT USE!

aaron.ballman wrote:
> Does using the fixed underlying type buy us something? As best I can tell, 
> this will introduce a fair amount of implicit promotions to `int` anyway.
Will it? I thought if I use the proper LLVM bitmask/flag enum thing it will 
work properly. For now, it is good for the bit flag debug print, because it 
only prints 8 numbers, not 32.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:90-91
+
+  void sanitise() {
+assert(Flags != MIX_Invalid && "sanitise() called on invalid bitvec");
+// TODO: There will be statements here in further extensions of the check.

aaron.ballman wrote:
> Heh, I don't know if we have any guidance on US English vs British English 
> spellings. ;-)
Reflexes, my bad. (Similarly how I put the pointer `*` to the left side, where 
Stroustrup intended. At least my local pre-commit hooks take care of that.) 
Will try to remember this.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:255
+  std::string NodeTypeName =
+  Node->getType().getAsString(Node->getASTContext().getPrintingPolicy());
+  StringRef CaptureName{NodeTypeName};

aaron.ballman wrote:
> Hmm, one downside to using the printing policy to get the node name is that 
> there can be alternative spellings for the same type that the user might 
> reasonably expect to be applied. e.g., if the user expects to ignore `bool` 
> datatypes and the printing policy wants to spell the type as `_Bool`, this 
> won't behave as expected.
But then the diagnostic is inconsistent with what the compiler is configured to 
output as diagnostics, isn't it? Can I stringify through Clang with some 
"default" printing policy?



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:402-403
+
+// Unfortunately, undersquiggly highlights are for some reason chewed up
+// and not respected by diagnostics from Clang-Tidy. :(
+diag(First->getLocation(), "the first parameter in the range is '%0'",

aaron.ballman wrote:
> Can you post a screenshot of what you mean?
Given

```
Diag << SourceRange{First->getSourceRange().getBegin(), 
Last->getSourceRange().getEnd()};
```

The diagnostic is still produced with only a `^` at the original 
`diag(Location)`, ignoring the fed `SourceRange`:

```
/home/whisperity/LLVM/Build/../llvm-project/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:21:18:
 warning: 3 adjacent parameters of 'redeclChain' of similar type ('int') are 
easily swapped by mistake [bugprone-easily-swappable-parameters]
void redeclChain(int I, int J, int K) {}
 ^
```

Instead of putting all the squigglies in as the range-location highlight, like 
how `Sema` can diagnose:

```
/home/whisperity/LLVM/Build/../llvm-project/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:21:18:
 warning: 3 adjacent parameters of 'redeclChain' of similar type ('int') are 
easily swapped by mistake [bugprone-easily-swappable-parameters]
void redeclChain(int I, int J, int K) {}
 ^~~
```

I would not want to put additional note tags in an otherwise already verbose 
output.

Back in 2019 I was investigating this issue specifically for another checker I 
was working on, but hit the wall... Somewhere deep inside where Tidy diagnostic 
stuff is translated and consumed to Clang stuff, it goes away. It shouldn't, 
because there are statements that seemingly copy the `Diag.Ranges` array over, 
but it still does not come out.

... EDIT: I found [[ 
http://lists.llvm.org/pipermail/cfe-dev/2019-October/063676.html | the relevant 
mailing list post ]].



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h:30
+  void storeOptions(ClangTidyOptions::OptionMap ) override;
+
+  /// The minimum length of an 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:58
+
+namespace {
+

Is there a need for the anonymous namespace? (We typically only use them when 
defining a class and try to make them as narrow in scope as possible, and use 
`static` for function declarations instead.)



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:67
+/// The language features involved in allowing the mix between two parameters.
+enum MixFlags : unsigned char {
+  MIX_Invalid = 0, //< Sentinel bit pattern. DO NOT USE!

Does using the fixed underlying type buy us something? As best I can tell, this 
will introduce a fair amount of implicit promotions to `int` anyway.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:90-91
+
+  void sanitise() {
+assert(Flags != MIX_Invalid && "sanitise() called on invalid bitvec");
+// TODO: There will be statements here in further extensions of the check.

Heh, I don't know if we have any guidance on US English vs British English 
spellings. ;-)



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:105
+
+  void sanitise() { Data.sanitise(); }
+  MixFlags flags() const { return Data.Flags; }





Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:176-177
+   const FunctionDecl *FD,
+   const std::size_t StartIndex) {
+  const std::size_t NumParams = FD->getNumParams();
+  assert(StartIndex < NumParams && "out of bounds for start");

Outside of field declarations, we typically only `const`-qualify 
pointer/reference types.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:177
+   const std::size_t StartIndex) {
+  const std::size_t NumParams = FD->getNumParams();
+  assert(StartIndex < NumParams && "out of bounds for start");

Some interesting test cases to consider: varargs functions and K C functions



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:196
+// parameters that can be messed up at a call site.
+decltype(Ret.Mixes) MixesOfIth;
+for (std::size_t J = StartIndex; J < I; ++J) {

Preferable to spell this type out to make it easier on someone reading the code 
to understand the type.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:255
+  std::string NodeTypeName =
+  Node->getType().getAsString(Node->getASTContext().getPrintingPolicy());
+  StringRef CaptureName{NodeTypeName};

Hmm, one downside to using the printing policy to get the node name is that 
there can be alternative spellings for the same type that the user might 
reasonably expect to be applied. e.g., if the user expects to ignore `bool` 
datatypes and the printing policy wants to spell the type as `_Bool`, this 
won't behave as expected.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:284
+/// The default value for the MinimumLength check option.
+static constexpr unsigned DefaultMinimumLength = 2;
+

It might be good to move this to a more prominent location since it's a default 
value.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:301
+ "Iterator", "inputit", "InputIt", "forwardit",
+ "FowardIt", "bidirit", "BidirIt", "constiterator",
+ "const_iterator", "Const_Iterator", "Constiterator",

If we're going to include `ForwardIt`, we probably want things like `RandomIt` 
as well?



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:302
+ "FowardIt", "bidirit", "BidirIt", "constiterator",
+ "const_iterator", "Const_Iterator", "Constiterator",
+ "ConstIterator"});

`reverse_iterator` and `reverse_const_iterator` too?

How about ranges?



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:402-403
+
+// Unfortunately, undersquiggly highlights are for some reason chewed up
+// and not respected by diagnostics from Clang-Tidy. :(
+diag(First->getLocation(), "the first parameter in the range is '%0'",

Can you post a screenshot of what you mean?



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h:30
+  void 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

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

  - Added a fallback case to also diagnose when Clang parsed the two types to 
be canonically the same (we will elaborate a few cases of such equivalences 
later, such as `typedef`s)
- Fixed Clang-Format and Clang-Tidy warnings in the patch


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/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 3}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""} \
+// RUN:  ]}' --
+
+int add(int Left, int Right) { return Left + Right; } // NO-WARN: Only 2 parameters.
+
+int magic(int Left, int Right, int X, int Y) { return 0; }
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 4 adjacent parameters of 'magic' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:15: note: the first parameter in the range is 'Left'
+// CHECK-MESSAGES: :[[@LINE-3]]:43: note: the last parameter in the range is 'Y'
+
+void multipleDistinctTypes(int I, int J, int K,
+   long L, long M,
+   double D, double E, double F) {}
+// CHECK-MESSAGES: :[[@LINE-3]]:28: warning: 3 adjacent parameters of 'multipleDistinctTypes' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-4]]:32: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-5]]:46: note: the last parameter in the range is 'K'
+// NO-WARN: The [long, long] range is length of 2.
+// CHECK-MESSAGES: :[[@LINE-5]]:28: warning: 3 adjacent parameters of 'multipleDistinctTypes' of similar type ('double')
+// CHECK-MESSAGES: :[[@LINE-6]]:35: note: the first parameter in the range is 'D'
+// CHECK-MESSAGES: :[[@LINE-7]]:55: note: the last parameter in the range is 'F'
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
@@ -0,0 +1,141 @@
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""} \
+// RUN:  ]}' --
+
+#define assert(X)
+
+void declaration(int Param, int Other); // NO-WARN: No chance to change this function.
+
+struct S {};
+
+S *allocate() { return nullptr; }   // NO-WARN: 1 parameter.
+void allocate(S **Out) {}   // NO-WARN: 1 parameter.
+bool operator<(const S , const S ) { return true; } // NO-WARN: Operator.
+
+void redeclChain(int, int, int);
+void redeclChain(int I, int, int);
+void redeclChain(int, int J, int);
+void redeclChain(int I, int J, int K) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 3 adjacent parameters of 'redeclChain' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:22: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:36: note: the last parameter in the range is 'K'
+
+void copyMany(S *Src, S *Dst, unsigned Num) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 2 adjacent parameters of 'copyMany' of similar type ('S *')
+// CHECK-MESSAGES: :[[@LINE-2]]:18: note: the first parameter in the range is 'Src'
+// CHECK-MESSAGES: :[[@LINE-3]]:26: note: 

[PATCH] D69560: [clang-tidy] Add 'bugprone-easily-swappable-parameters' check

[Whisperity via Phabricator via cfe-commits]

whisperity updated this revision to Diff 318220.
whisperity retitled this revision from "[clang-tidy] Add 
'experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type' 
check" to "[clang-tidy] Add 'bugprone-easily-swappable-parameters' check".
whisperity edited the summary of this revision.
whisperity removed a reviewer: baloghadamsoftware.
whisperity set the repository for this revision to rG LLVM Github Monorepo.
whisperity removed a subscriber: o.gyorgy.
whisperity added a comment.

Refactored the check and rebased it against "current" `master`. This version 
now has the tests rewritten and fixed. In addition, this patch only introduces 
the **very basic** frame of the check (i.e. only //strict type equality//), 
nothing more. This is so that review can be done in a more digestible way.


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/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 3}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""} \
+// RUN:  ]}' --
+
+int add(int Left, int Right) { return Left + Right; } // NO-WARN: Only 2 parameters.
+
+int magic(int Left, int Right, int X, int Y) { return 0; }
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 4 adjacent parameters of 'magic' of similar type ('int') are easily swapped by mistake [bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:15: note: the first parameter in the range is 'Left'
+// CHECK-MESSAGES: :[[@LINE-3]]:43: note: the last parameter in the range is 'Y'
+
+void multipleDistinctTypes(int I, int J, int K,
+   long L, long M,
+   double D, double E, double F) {}
+// CHECK-MESSAGES: :[[@LINE-3]]:28: warning: 3 adjacent parameters of 'multipleDistinctTypes' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-4]]:32: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-5]]:46: note: the last parameter in the range is 'K'
+// NO-WARN: The [long, long] range is length of 2.
+// CHECK-MESSAGES: :[[@LINE-5]]:28: warning: 3 adjacent parameters of 'multipleDistinctTypes' of similar type ('double')
+// CHECK-MESSAGES: :[[@LINE-6]]:35: note: the first parameter in the range is 'D'
+// CHECK-MESSAGES: :[[@LINE-7]]:55: note: the last parameter in the range is 'F'
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp
@@ -0,0 +1,112 @@
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""} \
+// RUN:  ]}' --
+
+#define assert(X)
+
+void declaration(int Param, int Other); // NO-WARN: No chance to change this function.
+
+struct S {};
+
+S *allocate() { return nullptr; }   // NO-WARN: 1 parameter.
+void allocate(S **Out) {}   // NO-WARN: 1 parameter.
+bool operator<(const S , const S ) { return true; } // NO-WARN: Operator.
+
+void redeclChain(int, int, int);
+void redeclChain(int I, int, int);
+void redeclChain(int, int J, int);
+void redeclChain(int I, int J, int K) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 3 adjacent parameters of 'redeclChain' of similar type ('int') are easily swapped by