[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Whisperity via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe33d0478831e: [clang-tidy] Extend 
bugprone-easily-swappable-parameters with mixability… (authored by 
whisperity).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75041

Files:
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.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-implicit-qualifiers.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.c
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.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-qualifiermixing.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
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
@@ -3,7 +3,8 @@
 // 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: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0} \
+// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0}, \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \
 // RUN:  ]}' -- -x c
 
 #define bool _Bool
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
@@ -3,7 +3,8 @@
 // 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: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 1} \
+// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 1}, \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \
 // RUN:  ]}' --
 
 typedef int MyInt1;
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.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-len3.cpp
@@ -3,7 +3,8 @@
 // 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: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0} \
+// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0}, \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \
 // RUN:  ]}' --
 
 int add(int Left, int Right) { return Left + Right; } // NO-WARN: Only 2 parameters.
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.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-len2.cpp
@@ -3,7 +3,8 @@
 // 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: {key: 

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Gabor Marton via Phabricator via cfe-commits]

martong accepted this revision.
martong added a comment.

Looks acceptable to me as well. Thanks!


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

https://reviews.llvm.org/D75041

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

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:249
+  /// the conversion sequence. This method does **NOT** return Begin and End.
+  SmallVector getInvolvedTypesInSequence() const {
+SmallVector Ret;

aaron.ballman wrote:
> Return a `SmallVectorImpl` so that the size of the vector doesn't 
> matter to callers?
Meh... that seems to only work if the context is polymorphic, like reference 
parameters or pointers... This tries to return by value and construct, which is 
not possible for `Impl`.


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

https://reviews.llvm.org/D75041

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

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Whisperity via Phabricator via cfe-commits]

whisperity updated this revision to Diff 354478.
whisperity marked 5 inline comments as done.
whisperity added a comment.

**NFC** Fix nits.

Let's have one final run with the buildbots, just in case.


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

https://reviews.llvm.org/D75041

Files:
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.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-implicit-qualifiers.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.c
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.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-qualifiermixing.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
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
@@ -3,7 +3,8 @@
 // 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: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0} \
+// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0}, \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \
 // RUN:  ]}' -- -x c
 
 #define bool _Bool
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
@@ -3,7 +3,8 @@
 // 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: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 1} \
+// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 1}, \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \
 // RUN:  ]}' --
 
 typedef int MyInt1;
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.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-len3.cpp
@@ -3,7 +3,8 @@
 // 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: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0} \
+// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0}, \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \
 // RUN:  ]}' --
 
 int add(int Left, int Right) { return Left + Right; } // NO-WARN: Only 2 parameters.
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.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-len2.cpp
@@ -3,7 +3,8 @@
 // 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: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0} \
+// RUN: 

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[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 D75041#2799036 , @whisperity wrote:

> In D75041#2799023 , @martong wrote:
>
>> Perhaps all conversion related logic should go into their own implementation 
>> file? Seems like it adds up to roughly 1000 lines.
>
> That's against the usual conventions of the project (1 TU for 1 check 
> implementation). The methods are grouped by namespace, you can fold it up in 
> your editor.

FWIW, there's not a hard rule for that -- we put other tidy logic into Utils 
files, for example. However, I think such a cleanup could be done post-commit 
as an NFC change as well.

It's taken me a while to convince myself that it's reasonable to reimplement so 
much of the conversion logic in clang-tidy, but I don't think it's reasonable 
to try to expose an API from Clang for its conversion logic to be reusable in 
this context. So despite my mild discomfort with how complex the logic is in 
this patch, I think it's an acceptable approach. LGTM, aside from some minor 
style nits.




Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:249
+  /// the conversion sequence. This method does **NOT** return Begin and End.
+  SmallVector getInvolvedTypesInSequence() const {
+SmallVector Ret;

Return a `SmallVectorImpl` so that the size of the vector doesn't 
matter to callers?



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:556
+static inline bool isUselessSugar(const Type *T) {
+  return isa(T);
+}

One thing that could get interesting here are attributed types, as those may be 
"useless" or "useful" sugars depending on the attribute. However, I think we 
can deal with that later when we have more real-world uses in front of us.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:962
+public:
+  /// The conversion assocaited with a conversion function, together with the
+  /// mixability flags of the conversion function's parameter or return type





Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:1491
+
+const auto  = [&](StringRef ToAdd) {
+  if (LastAddedType != ToAdd && ToAdd != SeqEndTypeStr) {





Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:1493
+  if (LastAddedType != ToAdd && ToAdd != SeqEndTypeStr) {
+OS << " -> '" << ToAdd << '\'';
+LastAddedType = ToAdd.str();





Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:1503
+if (LastAddedType != DestinationTypeAsDiagnosed) {
+  OS << " -> '" << DestinationTypeAsDiagnosed << '\'';
+  LastAddedType = DestinationTypeAsDiagnosed.str();




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75041

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

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Whisperity via Phabricator via cfe-commits]

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

**NC** Rebase, buildbots, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75041

Files:
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.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-implicit-qualifiers.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.c
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.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-qualifiermixing.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
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
@@ -3,7 +3,8 @@
 // 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: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0} \
+// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0}, \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \
 // RUN:  ]}' -- -x c
 
 #define bool _Bool
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
@@ -3,7 +3,8 @@
 // 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: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 1} \
+// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 1}, \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \
 // RUN:  ]}' --
 
 typedef int MyInt1;
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.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-len3.cpp
@@ -3,7 +3,8 @@
 // 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: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0} \
+// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0}, \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \
 // RUN:  ]}' --
 
 int add(int Left, int Right) { return Left + Right; } // NO-WARN: Only 2 parameters.
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.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-len2.cpp
@@ -3,7 +3,8 @@
 // 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: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0} \
+// RUN: {key: 

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Whisperity via Phabricator via cfe-commits]

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

(Uploaded the prerequisite, wrong patch here by accident.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75041

Files:
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.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-implicit-qualifiers.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.c
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.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-qualifiermixing.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
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
@@ -3,7 +3,8 @@
 // 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: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0} \
+// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0}, \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \
 // RUN:  ]}' -- -x c
 
 #define bool _Bool
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
@@ -3,7 +3,8 @@
 // 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: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 1} \
+// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 1}, \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \
 // RUN:  ]}' --
 
 typedef int MyInt1;
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.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-len3.cpp
@@ -3,7 +3,8 @@
 // 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: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0} \
+// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0}, \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \
 // RUN:  ]}' --
 
 int add(int Left, int Right) { return Left + Right; } // NO-WARN: Only 2 parameters.
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.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-len2.cpp
@@ -3,7 +3,8 @@
 // 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: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0} \
+// RUN: {key: 

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Whisperity via Phabricator via cfe-commits]

whisperity updated this revision to Diff 353975.
whisperity marked 5 inline comments as done.
whisperity added a comment.

**NFC**.

- Rebase and update for changes of base patch D69560 
.
- Remove the `<<=` and `%=` operators in favour of named functions.
- Elaborated the documentation for `ConversionSequence`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75041

Files:
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.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-qualifiermixing.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
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
@@ -2,7 +2,8 @@
 // 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: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: "bool;MyBool;struct U;MAKE_LOGICAL_TYPE(int)"}, \
+// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0} \
 // RUN:  ]}' -- -x c
 
 #define bool _Bool
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.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: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 1} \
+// RUN:  ]}' --
+
+typedef int MyInt1;
+typedef int MyInt2;
+using CInt = const int;
+using CMyInt1 = const MyInt1;
+using CMyInt2 = const MyInt2;
+
+void qualified1(int I, const int CI) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: 2 adjacent parameters of 'qualified1' of similar type are easily swapped by mistake [bugprone-easily-swappable-parameters]
+// CHECK-MESSAGES: :[[@LINE-2]]:21: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:34: note: the last parameter in the range is 'CI'
+// CHECK-MESSAGES: :[[@LINE-4]]:24: note: 'int' and 'const int' parameters accept and bind the same kind of values
+
+void qualified2(int I, volatile int VI) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: 2 adjacent parameters of 'qualified2' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:21: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:37: note: the last parameter in the range is 'VI'
+// CHECK-MESSAGES: :[[@LINE-4]]:24: note: 'int' and 'volatile int' parameters accept and bind the same kind of values
+
+void qualified3(int I, const volatile int CVI) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: 2 adjacent parameters of 'qualified3' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:21: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-3]]:43: note: the last parameter in the range is 'CVI'
+// CHECK-MESSAGES: :[[@LINE-4]]:24: note: 'int' and 'const volatile int' parameters accept and bind the same kind of values
+
+void qualified4(int *IP, const int *CIP) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: 2 adjacent parameters of 'qualified4' of similar type are
+// CHECK-MESSAGES: :[[@LINE-2]]:22: note: the first parameter in the range is 'IP'
+// CHECK-MESSAGES: :[[@LINE-3]]:37: note: the last parameter in the range is 'CIP'
+// CHECK-MESSAGES: :[[@LINE-4]]:26: note: 'int *' and 'const int *' parameters accept and bind the same kind of values
+
+void qualified5(const int CI, const long CL) {} // NO-WARN: Not the same type
+

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[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:657-667
+  // Certain kinds unfortunately need to be side-stepped for canonical type
+  // matching.
+  if (LType->getAs() || RType->getAs()) {
+// Unfortunately, the canonical type of a function pointer becomes the
+// same even if exactly one is "noexcept" and the other isn't, making us
+// give a false positive report irrespective of implicit conversions.
+LLVM_DEBUG(llvm::dbgs()

whisperity wrote:
> whisperity wrote:
> > martong wrote:
> > > aaron.ballman wrote:
> > > > whisperity wrote:
> > > > > @aaron.ballman Getting ahead of the curve here. I understand this is 
> > > > > ugly. Unfortunately, no matter how much I read the standard, I don't 
> > > > > get anything of "canonical type" mentioned, it seems to me this 
> > > > > concept is something inherent to the model of Clang.
> > > > > 
> > > > > Basically why this is here: imagine a `void (*)() noexcept` and a 
> > > > > `void (*)()`. One's `noexcept`, the other isn't. Inside the AST, this 
> > > > > is a `ParenType` of a `PointerType` to a `FunctionProtoType`. There 
> > > > > exists a //one-way// implicit conversion from the `noexcept` to the 
> > > > > non-noexcept ("noexceptness can be discarded", similarly to how 
> > > > > "constness can be gained")
> > > > > Unfortunately, because this is a one-way implicit conversion, it 
> > > > > won't return on the code path earlier for implicit conversions.
> > > > > 
> > > > > Now because of this, the recursive descent in our code will reach the 
> > > > > point when the two innermost `FunctionProtoType`s are in our hands. 
> > > > > As a fallback case, we simply ask Clang "Hey, do //you// think these 
> > > > > two are the same?". And for some weird reason, Clang will say 
> > > > > "Yes."... While one of them is a `noexcept` function and the other 
> > > > > one isn't.
> > > > > 
> > > > > I do not know the innards of the AST well enough to even have a 
> > > > > glimpse of whether or not this is a bug. So that's the reason why I 
> > > > > went ahead and implemented this side-stepping logic. Basically, as 
> > > > > the comment says, it'lll **force** the information of "No matter what 
> > > > > you do, do NOT consider these mixable!" back up the recursion chain, 
> > > > > and handle it appropriately later.
> > > > > Unfortunately, no matter how much I read the standard, I don't get 
> > > > > anything of "canonical type" mentioned, it seems to me this concept 
> > > > > is something inherent to the model of Clang.
> > > > 
> > > > It is more of a Clang-centric concept. Basically, a canonical type is 
> > > > one that's had all of the typedefs stripped off it.
> > > > 
> > > > > Now because of this, the recursive descent in our code will reach the 
> > > > > point when the two innermost FunctionProtoTypes are in our hands. As 
> > > > > a fallback case, we simply ask Clang "Hey, do you think these two are 
> > > > > the same?". And for some weird reason, Clang will say "Yes."... While 
> > > > > one of them is a noexcept function and the other one isn't.
> > > > 
> > > > I think a confounding factor in this area is that `noexcept` did not 
> > > > used to be part of the function type until one day it started being a 
> > > > part of the function type. So my guess is that this is fallout from 
> > > > this sort of thing: https://godbolt.org/z/EYfj8z (which may be worth 
> > > > keeping in mind when working on the check).
> > > About `noexcept`: we've faced a similar problem in the `ASTImporter` 
> > > library. We could not import a noexcept function's definition if we 
> > > already had one without the noexcept specifier. 
> > > 
> > > Thus, in `ASTStructuralEquivalence.cpp` we do differentiate the function 
> > > types based on their noexcept specifier (and we even check the noexcept 
> > > expression).:
> > > ```
> > > TEST_F(StructuralEquivalenceFunctionTest, Noexcept) {
> > >   auto t = makeNamedDecls("void foo();",
> > >   "void foo() noexcept;", Lang_CXX11);
> > >   EXPECT_FALSE(testStructuralMatch(t));
> > > }
> > > TEST_F(StructuralEquivalenceFunctionTest, NoexceptNonMatch) {
> > >   auto t = makeNamedDecls("void foo() noexcept(false);",
> > >   "void foo() noexcept(true);", Lang_CXX11);
> > >   EXPECT_FALSE(testStructuralMatch(t));
> > > }
> > > ```
> > > 
> > > May be in these special cases it would worth to reuse the 
> > > ASTStructuralEquivalenceContext class?
> > Definitely, good catch, @martong, thank you very much! @aaron.ballman, what 
> > do you think? If I see this right, `StructuralEquivalenceContext` is part 
> > of `libClangAST` so should be readily available.
> > 
> > The only issue I'm seeing is that this class takes non-**`const`** 
> > `ASTContext` and `Decl` nodes...
> Well, I started looking into putting up 

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Whisperity via Phabricator via cfe-commits]

whisperity marked 5 inline comments as done.
whisperity added a comment.

In D75041#2799023 , @martong wrote:

> Perhaps all conversion related logic should go into their own implementation 
> file? Seems like it adds up to roughly 1000 lines.

That's against the usual conventions of the project (1 TU for 1 check 
implementation). The methods are grouped by namespace, you can fold it up in 
your editor.




Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:550-551
+  if (isUselessSugar(LType.getTypePtr())) {
+LLVM_DEBUG(llvm::dbgs()
+   << "--- calculateMixability. LHS is useless sugar.\n");
 return calculateMixability(Check, LType.getSingleStepDesugaredType(Ctx),

martong wrote:
> Is this still WIP or do you use the DEBUG printouts in the tests? If not then 
> could you please create a Diff without the DEBUG printouts, that could 
> increase readability and ease the review.
As discussed earlier (and perhaps in the previous patches), these debug 
printouts are needed and shall stay here. The output is not used //in 
automation//, but it's intended for people who might pick up on further 
developing the check later. The recursive descent is too complex to be 
handleable in your mind without the debug information.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75041

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

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:550-551
+  if (isUselessSugar(LType.getTypePtr())) {
+LLVM_DEBUG(llvm::dbgs()
+   << "--- calculateMixability. LHS is useless sugar.\n");
 return calculateMixability(Check, LType.getSingleStepDesugaredType(Ctx),

Is this still WIP or do you use the DEBUG printouts in the tests? If not then 
could you please create a Diff without the DEBUG printouts, that could increase 
readability and ease the review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75041

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

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Perhaps all conversion related logic should go into their own implementation 
file? Seems like it adds up to roughly 1000 lines.




Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:1171
+   "conversion operator.\n");
+ImplicitSeq <<= ConversionOperatorResult.getValue();
+WorkType = ImplicitSeq.getTypeAfterUserDefinedConversion();

I think that these overloaded operators are not elevating the readability, 
since we are not dealing with mathematical classes.
IMHO, we could simply use `update` or something that is less crypto.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75041

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

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:122
 
 #define FB(Name, K) MIX_##Name = (1ull << (K##ull - 1ull))
 

whisperity wrote:
> martong wrote:
> > FB stands for FunnyBitmask? Could you please either describe that in a 
> > comment or just spell it out?
> FlagBit. @alexfh suggested in the base check to use hexa literals. I'm not 
> too sold about that, but maybe we can cut the macro out and keep the bit 
> shift instructions in. Given that the check has more or less earned its final 
> form (for now), we know how many bits we need!
FlagBit, I like it, perhaps we should rename `FB` to `FlagBit`?



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:182
+  /// The intermediate type after the first Standard Conversion Sequence.
+  QualType AfterPreStd;
+

whisperity wrote:
> martong wrote:
> > Maybe it's just me, but AfterPre sounds ambiguous and AfterPost seems 
> > redundant.
> Should I use "AfterFirstStandard" or "AfterFirstStd"?
> 
> AfterPost isn't redundant. An implicit conversion sequence might be of **3 
> steps**. Consider in a hypothetical `void f(Complex x, const double d)`.
> 
> `Complex` ---(PRE: std qual adjustment)--> `const Complex` ---(UD: user 
> defined conv)--> `double` ---(POST: std qual adjustment)--> `const double`
> 
> And because in case of many conversions a lot of combinations need to be 
> checked, we can't have `AfterPost` be the same as `End`. First, because of 
> the combinations, second, because of the //other// things the check is doing.
> 
> `void g(ComplexTypedef CT, ConstDoubleTypedef CDT);`
> 
> In this case, `Start` and `End` are the typedefs, and the inner sequence is 
> the same as before. And in order to generate the diagnostic, we also need to 
> have **both** pieces of information.
Thanks for this explanation, it makes clearer! However, this highlights that a 
way better description (in the form of comments) is needed for these `QualType` 
members. Especially this line could really increase the readability.
```
Complex ---(PRE: std qual adjustment)--> const Complex ---(UD: user defined 
conv)--> double ---(POST: std qual adjustment)--> const double
```

> In this case, Start and End are the typedefs, and the inner sequence is the 
> same as before. And in order to generate the diagnostic, we also need to have 
> both pieces of information.

I think it would be useful to document the cases with examples when `End` is 
not equal to `AfterPost` and such.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:210
+  /// the conversion sequence. This method does **NOT** return Begin and End.
+  SmallVector getInvolvedTypesInSequence() const {
+SmallVector Ret;

whisperity wrote:
> martong wrote:
> > So, we can never return with a vector with size > 4?
> `N` in `SmallVector` only specifies the pre-allocated smallness size. 
> AFAIK, if you have >N elements, it will just put the vector out onto the heap.
I meant the number 4, not SmallVector. So, is there a case when the conversion 
sequence is longer than 4? If it can be longer, then where do you store the 
types, you have 4 `QualType` members if I am not wrong. (Also, I am not an 
expert of conversions.) 

To be honest, it is so terrible that we cannot reuse [[ 
https://clang.llvm.org/doxygen/classclang_1_1StandardConversionSequence.html | 
clang::StandardConversionSequence ]]



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h:42
 
-  /// Whether to consider an unqualified and a qualified type mixable.
+  /// Whether to consider differently qualified versions of the same type
+  /// mixable.

whisperity wrote:
> martong wrote:
> > "qualified"
> > Do you consider `volatile` here as well, or just `const`?
> Const, volatile, and in C mode, restrict, too.
Great!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75041

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

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:122
 
 #define FB(Name, K) MIX_##Name = (1ull << (K##ull - 1ull))
 

martong wrote:
> FB stands for FunnyBitmask? Could you please either describe that in a 
> comment or just spell it out?
FlagBit. @alexfh suggested in the base check to use hexa literals. I'm not too 
sold about that, but maybe we can cut the macro out and keep the bit shift 
instructions in. Given that the check has more or less earned its final form 
(for now), we know how many bits we need!



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:182
+  /// The intermediate type after the first Standard Conversion Sequence.
+  QualType AfterPreStd;
+

martong wrote:
> Maybe it's just me, but AfterPre sounds ambiguous and AfterPost seems 
> redundant.
Should I use "AfterFirstStandard" or "AfterFirstStd"?

AfterPost isn't redundant. An implicit conversion sequence might be of **3 
steps**. Consider in a hypothetical `void f(Complex x, const double d)`.

`Complex` ---(PRE: std qual adjustment)--> `const Complex` ---(UD: user defined 
conv)--> `double` ---(POST: std qual adjustment)--> `const double`

And because in case of many conversions a lot of combinations need to be 
checked, we can't have `AfterPost` be the same as `End`. First, because of the 
combinations, second, because of the //other// things the check is doing.

`void g(ComplexTypedef CT, ConstDoubleTypedef CDT);`

In this case, `Start` and `End` are the typedefs, and the inner sequence is the 
same as before. And in order to generate the diagnostic, we also need to have 
**both** pieces of information.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:210
+  /// the conversion sequence. This method does **NOT** return Begin and End.
+  SmallVector getInvolvedTypesInSequence() const {
+SmallVector Ret;

martong wrote:
> So, we can never return with a vector with size > 4?
`N` in `SmallVector` only specifies the pre-allocated smallness size. 
AFAIK, if you have >N elements, it will just put the vector out onto the heap.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h:42
 
-  /// Whether to consider an unqualified and a qualified type mixable.
+  /// Whether to consider differently qualified versions of the same type
+  /// mixable.

martong wrote:
> "qualified"
> Do you consider `volatile` here as well, or just `const`?
Const, volatile, and in C mode, restrict, too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75041

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

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:657-667
+  // Certain kinds unfortunately need to be side-stepped for canonical type
+  // matching.
+  if (LType->getAs() || RType->getAs()) {
+// Unfortunately, the canonical type of a function pointer becomes the
+// same even if exactly one is "noexcept" and the other isn't, making us
+// give a false positive report irrespective of implicit conversions.
+LLVM_DEBUG(llvm::dbgs()

whisperity wrote:
> martong wrote:
> > aaron.ballman wrote:
> > > whisperity wrote:
> > > > @aaron.ballman Getting ahead of the curve here. I understand this is 
> > > > ugly. Unfortunately, no matter how much I read the standard, I don't 
> > > > get anything of "canonical type" mentioned, it seems to me this concept 
> > > > is something inherent to the model of Clang.
> > > > 
> > > > Basically why this is here: imagine a `void (*)() noexcept` and a `void 
> > > > (*)()`. One's `noexcept`, the other isn't. Inside the AST, this is a 
> > > > `ParenType` of a `PointerType` to a `FunctionProtoType`. There exists a 
> > > > //one-way// implicit conversion from the `noexcept` to the non-noexcept 
> > > > ("noexceptness can be discarded", similarly to how "constness can be 
> > > > gained")
> > > > Unfortunately, because this is a one-way implicit conversion, it won't 
> > > > return on the code path earlier for implicit conversions.
> > > > 
> > > > Now because of this, the recursive descent in our code will reach the 
> > > > point when the two innermost `FunctionProtoType`s are in our hands. As 
> > > > a fallback case, we simply ask Clang "Hey, do //you// think these two 
> > > > are the same?". And for some weird reason, Clang will say "Yes."... 
> > > > While one of them is a `noexcept` function and the other one isn't.
> > > > 
> > > > I do not know the innards of the AST well enough to even have a glimpse 
> > > > of whether or not this is a bug. So that's the reason why I went ahead 
> > > > and implemented this side-stepping logic. Basically, as the comment 
> > > > says, it'lll **force** the information of "No matter what you do, do 
> > > > NOT consider these mixable!" back up the recursion chain, and handle it 
> > > > appropriately later.
> > > > Unfortunately, no matter how much I read the standard, I don't get 
> > > > anything of "canonical type" mentioned, it seems to me this concept is 
> > > > something inherent to the model of Clang.
> > > 
> > > It is more of a Clang-centric concept. Basically, a canonical type is one 
> > > that's had all of the typedefs stripped off it.
> > > 
> > > > Now because of this, the recursive descent in our code will reach the 
> > > > point when the two innermost FunctionProtoTypes are in our hands. As a 
> > > > fallback case, we simply ask Clang "Hey, do you think these two are the 
> > > > same?". And for some weird reason, Clang will say "Yes."... While one 
> > > > of them is a noexcept function and the other one isn't.
> > > 
> > > I think a confounding factor in this area is that `noexcept` did not used 
> > > to be part of the function type until one day it started being a part of 
> > > the function type. So my guess is that this is fallout from this sort of 
> > > thing: https://godbolt.org/z/EYfj8z (which may be worth keeping in mind 
> > > when working on the check).
> > About `noexcept`: we've faced a similar problem in the `ASTImporter` 
> > library. We could not import a noexcept function's definition if we already 
> > had one without the noexcept specifier. 
> > 
> > Thus, in `ASTStructuralEquivalence.cpp` we do differentiate the function 
> > types based on their noexcept specifier (and we even check the noexcept 
> > expression).:
> > ```
> > TEST_F(StructuralEquivalenceFunctionTest, Noexcept) {
> >   auto t = makeNamedDecls("void foo();",
> >   "void foo() noexcept;", Lang_CXX11);
> >   EXPECT_FALSE(testStructuralMatch(t));
> > }
> > TEST_F(StructuralEquivalenceFunctionTest, NoexceptNonMatch) {
> >   auto t = makeNamedDecls("void foo() noexcept(false);",
> >   "void foo() noexcept(true);", Lang_CXX11);
> >   EXPECT_FALSE(testStructuralMatch(t));
> > }
> > ```
> > 
> > May be in these special cases it would worth to reuse the 
> > ASTStructuralEquivalenceContext class?
> Definitely, good catch, @martong, thank you very much! @aaron.ballman, what 
> do you think? If I see this right, `StructuralEquivalenceContext` is part of 
> `libClangAST` so should be readily available.
> 
> The only issue I'm seeing is that this class takes non-**`const`** 
> `ASTContext` and `Decl` nodes...
Well, I started looking into putting up `const` whereever possible into the 
aforementioned type, but I hit a hurdle. When checking equivalence of records, 
the algorithm tries to ASTImport-complete the definition of a record if it's 
not fully defined yet... which is 

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:122
 
 #define FB(Name, K) MIX_##Name = (1ull << (K##ull - 1ull))
 

FB stands for FunnyBitmask? Could you please either describe that in a comment 
or just spell it out?



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:182
+  /// The intermediate type after the first Standard Conversion Sequence.
+  QualType AfterPreStd;
+

Maybe it's just me, but AfterPre sounds ambiguous and AfterPost seems redundant.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:210
+  /// the conversion sequence. This method does **NOT** return Begin and End.
+  SmallVector getInvolvedTypesInSequence() const {
+SmallVector Ret;

So, we can never return with a vector with size > 4?



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h:42
 
-  /// Whether to consider an unqualified and a qualified type mixable.
+  /// Whether to consider differently qualified versions of the same type
+  /// mixable.

"qualified"
Do you consider `volatile` here as well, or just `const`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75041

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

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:657-667
+  // Certain kinds unfortunately need to be side-stepped for canonical type
+  // matching.
+  if (LType->getAs() || RType->getAs()) {
+// Unfortunately, the canonical type of a function pointer becomes the
+// same even if exactly one is "noexcept" and the other isn't, making us
+// give a false positive report irrespective of implicit conversions.
+LLVM_DEBUG(llvm::dbgs()

martong wrote:
> aaron.ballman wrote:
> > whisperity wrote:
> > > @aaron.ballman Getting ahead of the curve here. I understand this is 
> > > ugly. Unfortunately, no matter how much I read the standard, I don't get 
> > > anything of "canonical type" mentioned, it seems to me this concept is 
> > > something inherent to the model of Clang.
> > > 
> > > Basically why this is here: imagine a `void (*)() noexcept` and a `void 
> > > (*)()`. One's `noexcept`, the other isn't. Inside the AST, this is a 
> > > `ParenType` of a `PointerType` to a `FunctionProtoType`. There exists a 
> > > //one-way// implicit conversion from the `noexcept` to the non-noexcept 
> > > ("noexceptness can be discarded", similarly to how "constness can be 
> > > gained")
> > > Unfortunately, because this is a one-way implicit conversion, it won't 
> > > return on the code path earlier for implicit conversions.
> > > 
> > > Now because of this, the recursive descent in our code will reach the 
> > > point when the two innermost `FunctionProtoType`s are in our hands. As a 
> > > fallback case, we simply ask Clang "Hey, do //you// think these two are 
> > > the same?". And for some weird reason, Clang will say "Yes."... While one 
> > > of them is a `noexcept` function and the other one isn't.
> > > 
> > > I do not know the innards of the AST well enough to even have a glimpse 
> > > of whether or not this is a bug. So that's the reason why I went ahead 
> > > and implemented this side-stepping logic. Basically, as the comment says, 
> > > it'lll **force** the information of "No matter what you do, do NOT 
> > > consider these mixable!" back up the recursion chain, and handle it 
> > > appropriately later.
> > > Unfortunately, no matter how much I read the standard, I don't get 
> > > anything of "canonical type" mentioned, it seems to me this concept is 
> > > something inherent to the model of Clang.
> > 
> > It is more of a Clang-centric concept. Basically, a canonical type is one 
> > that's had all of the typedefs stripped off it.
> > 
> > > Now because of this, the recursive descent in our code will reach the 
> > > point when the two innermost FunctionProtoTypes are in our hands. As a 
> > > fallback case, we simply ask Clang "Hey, do you think these two are the 
> > > same?". And for some weird reason, Clang will say "Yes."... While one of 
> > > them is a noexcept function and the other one isn't.
> > 
> > I think a confounding factor in this area is that `noexcept` did not used 
> > to be part of the function type until one day it started being a part of 
> > the function type. So my guess is that this is fallout from this sort of 
> > thing: https://godbolt.org/z/EYfj8z (which may be worth keeping in mind 
> > when working on the check).
> About `noexcept`: we've faced a similar problem in the `ASTImporter` library. 
> We could not import a noexcept function's definition if we already had one 
> without the noexcept specifier. 
> 
> Thus, in `ASTStructuralEquivalence.cpp` we do differentiate the function 
> types based on their noexcept specifier (and we even check the noexcept 
> expression).:
> ```
> TEST_F(StructuralEquivalenceFunctionTest, Noexcept) {
>   auto t = makeNamedDecls("void foo();",
>   "void foo() noexcept;", Lang_CXX11);
>   EXPECT_FALSE(testStructuralMatch(t));
> }
> TEST_F(StructuralEquivalenceFunctionTest, NoexceptNonMatch) {
>   auto t = makeNamedDecls("void foo() noexcept(false);",
>   "void foo() noexcept(true);", Lang_CXX11);
>   EXPECT_FALSE(testStructuralMatch(t));
> }
> ```
> 
> May be in these special cases it would worth to reuse the 
> ASTStructuralEquivalenceContext class?
Definitely, good catch, @martong, thank you very much! @aaron.ballman, what do 
you think? If I see this right, `StructuralEquivalenceContext` is part of 
`libClangAST` so should be readily available.

The only issue I'm seeing is that this class takes non-**`const`** `ASTContext` 
and `Decl` nodes...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75041

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

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:657-667
+  // Certain kinds unfortunately need to be side-stepped for canonical type
+  // matching.
+  if (LType->getAs() || RType->getAs()) {
+// Unfortunately, the canonical type of a function pointer becomes the
+// same even if exactly one is "noexcept" and the other isn't, making us
+// give a false positive report irrespective of implicit conversions.
+LLVM_DEBUG(llvm::dbgs()

aaron.ballman wrote:
> whisperity wrote:
> > @aaron.ballman Getting ahead of the curve here. I understand this is ugly. 
> > Unfortunately, no matter how much I read the standard, I don't get anything 
> > of "canonical type" mentioned, it seems to me this concept is something 
> > inherent to the model of Clang.
> > 
> > Basically why this is here: imagine a `void (*)() noexcept` and a `void 
> > (*)()`. One's `noexcept`, the other isn't. Inside the AST, this is a 
> > `ParenType` of a `PointerType` to a `FunctionProtoType`. There exists a 
> > //one-way// implicit conversion from the `noexcept` to the non-noexcept 
> > ("noexceptness can be discarded", similarly to how "constness can be 
> > gained")
> > Unfortunately, because this is a one-way implicit conversion, it won't 
> > return on the code path earlier for implicit conversions.
> > 
> > Now because of this, the recursive descent in our code will reach the point 
> > when the two innermost `FunctionProtoType`s are in our hands. As a fallback 
> > case, we simply ask Clang "Hey, do //you// think these two are the same?". 
> > And for some weird reason, Clang will say "Yes."... While one of them is a 
> > `noexcept` function and the other one isn't.
> > 
> > I do not know the innards of the AST well enough to even have a glimpse of 
> > whether or not this is a bug. So that's the reason why I went ahead and 
> > implemented this side-stepping logic. Basically, as the comment says, 
> > it'lll **force** the information of "No matter what you do, do NOT consider 
> > these mixable!" back up the recursion chain, and handle it appropriately 
> > later.
> > Unfortunately, no matter how much I read the standard, I don't get anything 
> > of "canonical type" mentioned, it seems to me this concept is something 
> > inherent to the model of Clang.
> 
> It is more of a Clang-centric concept. Basically, a canonical type is one 
> that's had all of the typedefs stripped off it.
> 
> > Now because of this, the recursive descent in our code will reach the point 
> > when the two innermost FunctionProtoTypes are in our hands. As a fallback 
> > case, we simply ask Clang "Hey, do you think these two are the same?". And 
> > for some weird reason, Clang will say "Yes."... While one of them is a 
> > noexcept function and the other one isn't.
> 
> I think a confounding factor in this area is that `noexcept` did not used to 
> be part of the function type until one day it started being a part of the 
> function type. So my guess is that this is fallout from this sort of thing: 
> https://godbolt.org/z/EYfj8z (which may be worth keeping in mind when working 
> on the check).
About `noexcept`: we've faced a similar problem in the `ASTImporter` library. 
We could not import a noexcept function's definition if we already had one 
without the noexcept specifier. 

Thus, in `ASTStructuralEquivalence.cpp` we do differentiate the function types 
based on their noexcept specifier (and we even check the noexcept expression).:
```
TEST_F(StructuralEquivalenceFunctionTest, Noexcept) {
  auto t = makeNamedDecls("void foo();",
  "void foo() noexcept;", Lang_CXX11);
  EXPECT_FALSE(testStructuralMatch(t));
}
TEST_F(StructuralEquivalenceFunctionTest, NoexceptNonMatch) {
  auto t = makeNamedDecls("void foo() noexcept(false);",
  "void foo() noexcept(true);", Lang_CXX11);
  EXPECT_FALSE(testStructuralMatch(t));
}
```

May be in these special cases it would worth to reuse the 
ASTStructuralEquivalenceContext class?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75041

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

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Bump.  This is the last part that's still unreviewed and also the most complex.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75041

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

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Whisperity via Phabricator via cfe-commits]

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

- Fixed a few nits.
- Rebased, and added highlighting (under-squiggly) the parameter/return type of 
conversion operators for extra emphasis.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75041

Files:
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.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-implicit-qualifiers.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.c
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.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-qualifiermixing.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
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
@@ -3,7 +3,8 @@
 // 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: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0} \
+// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0}, \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \
 // RUN:  ]}' -- -x c
 
 #define bool _Bool
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
@@ -3,7 +3,8 @@
 // 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: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 1} \
+// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 1}, \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \
 // RUN:  ]}' --
 
 typedef int MyInt1;
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.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-len3.cpp
@@ -3,7 +3,8 @@
 // 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: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0} \
+// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0}, \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \
 // RUN:  ]}' --
 
 int add(int Left, int Right) { return Left + Right; } // NO-WARN: Only 2 parameters.
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.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-len2.cpp
@@ -3,7 +3,8 @@
 // RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
 // RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
 // RUN: {key: 

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:203
+SmallVector Ret;
+const auto  = [](QualType QT) {
+  if (QT.isNull())





Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:207
+
+  LLVM_DEBUG(QT.dump());
+

whisperity wrote:
> Actually this is one of those debug prints that should be removed and 
> remained in here by accident.
Agreed, this one should be removed.


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

https://reviews.llvm.org/D75041

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

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Whisperity via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:377
+
+  bool indicatesMixability() const { return Flags > MIX_None; }
+

aaron.ballman wrote:
> Should this be `MIX_WorkaroundDisableCanonicalEquivalence` instead of 
> `MIX_None`?
In this patch I changed the value of `None` to be `2`, and `Workaround...` 
became `1`. So everything above `None` means it's a valid mixability (somehow), 
of course only after `sanitize()` so `Workaround... | CanonicalType` becomes 
`None`.


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

https://reviews.llvm.org/D75041

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

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:377
+
+  bool indicatesMixability() const { return Flags > MIX_None; }
+

Should this be `MIX_WorkaroundDisableCanonicalEquivalence` instead of 
`MIX_None`?



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:486-487
+static MixData
+approximateImplicitConversion(const TheCheck , const QualType LType,
+  const QualType RType, const ASTContext ,
+  ImplicitConversionModellingMode ImplicitMode);





Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:500-501
+static MixData
+calculateMixability(const TheCheck , const QualType LType,
+const QualType RType, const ASTContext ,
+ImplicitConversionModellingMode ImplicitMode) {





Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:520
   // user to understand.
-  if (isa(LType.getTypePtr())) {
-LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. LHS is ParenType.\n");
+  if (isa(LType.getTypePtr())) {
+LLVM_DEBUG(llvm::dbgs()

What about a `DecayedType`?



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:657-667
+  // Certain kinds unfortunately need to be side-stepped for canonical type
+  // matching.
+  if (LType->getAs() || RType->getAs()) {
+// Unfortunately, the canonical type of a function pointer becomes the
+// same even if exactly one is "noexcept" and the other isn't, making us
+// give a false positive report irrespective of implicit conversions.
+LLVM_DEBUG(llvm::dbgs()

whisperity wrote:
> @aaron.ballman Getting ahead of the curve here. I understand this is ugly. 
> Unfortunately, no matter how much I read the standard, I don't get anything 
> of "canonical type" mentioned, it seems to me this concept is something 
> inherent to the model of Clang.
> 
> Basically why this is here: imagine a `void (*)() noexcept` and a `void 
> (*)()`. One's `noexcept`, the other isn't. Inside the AST, this is a 
> `ParenType` of a `PointerType` to a `FunctionProtoType`. There exists a 
> //one-way// implicit conversion from the `noexcept` to the non-noexcept 
> ("noexceptness can be discarded", similarly to how "constness can be gained")
> Unfortunately, because this is a one-way implicit conversion, it won't return 
> on the code path earlier for implicit conversions.
> 
> Now because of this, the recursive descent in our code will reach the point 
> when the two innermost `FunctionProtoType`s are in our hands. As a fallback 
> case, we simply ask Clang "Hey, do //you// think these two are the same?". 
> And for some weird reason, Clang will say "Yes."... While one of them is a 
> `noexcept` function and the other one isn't.
> 
> I do not know the innards of the AST well enough to even have a glimpse of 
> whether or not this is a bug. So that's the reason why I went ahead and 
> implemented this side-stepping logic. Basically, as the comment says, it'lll 
> **force** the information of "No matter what you do, do NOT consider these 
> mixable!" back up the recursion chain, and handle it appropriately later.
> Unfortunately, no matter how much I read the standard, I don't get anything 
> of "canonical type" mentioned, it seems to me this concept is something 
> inherent to the model of Clang.

It is more of a Clang-centric concept. Basically, a canonical type is one 
that's had all of the typedefs stripped off it.

> Now because of this, the recursive descent in our code will reach the point 
> when the two innermost FunctionProtoTypes are in our hands. As a fallback 
> case, we simply ask Clang "Hey, do you think these two are the same?". And 
> for some weird reason, Clang will say "Yes."... While one of them is a 
> noexcept function and the other one isn't.

I think a confounding factor in this area is that `noexcept` did not used to be 
part of the function type until one day it started being a part of the function 
type. So my guess is that this is fallout from this sort of thing: 
https://godbolt.org/z/EYfj8z (which may be worth keeping in mind when working 
on the check).



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:758
+approximateStandardConversionSequence(const TheCheck ,
+  const QualType From, const QualType To,
+  const ASTContext ) {




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

https://reviews.llvm.org/D75041

___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Whisperity via Phabricator via cfe-commits]

whisperity added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:207
+
+  LLVM_DEBUG(QT.dump());
+

Actually this is one of those debug prints that should be removed and remained 
in here by accident.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:657-667
+  // Certain kinds unfortunately need to be side-stepped for canonical type
+  // matching.
+  if (LType->getAs() || RType->getAs()) {
+// Unfortunately, the canonical type of a function pointer becomes the
+// same even if exactly one is "noexcept" and the other isn't, making us
+// give a false positive report irrespective of implicit conversions.
+LLVM_DEBUG(llvm::dbgs()

@aaron.ballman Getting ahead of the curve here. I understand this is ugly. 
Unfortunately, no matter how much I read the standard, I don't get anything of 
"canonical type" mentioned, it seems to me this concept is something inherent 
to the model of Clang.

Basically why this is here: imagine a `void (*)() noexcept` and a `void (*)()`. 
One's `noexcept`, the other isn't. Inside the AST, this is a `ParenType` of a 
`PointerType` to a `FunctionProtoType`. There exists a //one-way// implicit 
conversion from the `noexcept` to the non-noexcept ("noexceptness can be 
discarded", similarly to how "constness can be gained")
Unfortunately, because this is a one-way implicit conversion, it won't return 
on the code path earlier for implicit conversions.

Now because of this, the recursive descent in our code will reach the point 
when the two innermost `FunctionProtoType`s are in our hands. As a fallback 
case, we simply ask Clang "Hey, do //you// think these two are the same?". And 
for some weird reason, Clang will say "Yes."... While one of them is a 
`noexcept` function and the other one isn't.

I do not know the innards of the AST well enough to even have a glimpse of 
whether or not this is a bug. So that's the reason why I went ahead and 
implemented this side-stepping logic. Basically, as the comment says, it'lll 
**force** the information of "No matter what you do, do NOT consider these 
mixable!" back up the recursion chain, and handle it appropriately later.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.cpp:250-260
+using NonThrowingFunction = void (*)() noexcept;
+
+struct NoexceptMaker {
+  NoexceptMaker(void (*ThrowingFunction)());
+  // Need to use a typedef here because
+  // "conversion function cannot convert to a function type".
+  // operator (void (*)() noexcept) () const;

Now here the warning is justified, however. All these disinfectant-stinking 
examples are to ensure that. In this case, in our hands, on the first level, we 
got the function pointer, and the class. And it **is** the implicit conversion 
modeller that will eventually ask "Can I give that noexcept function to that 
constructor taking a non-noexcept?" and in that case, on the level of function 
pointers, the answer is //yes//, so there will be a warning made.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp:336-337
+
+void functionPrototypeLosesNoexcept(void (*NonThrowing)() noexcept, void 
(*Throwing)()) {}
+// NO-WARN: This call cannot be swapped, even if "getCanonicalType()" believes 
otherwise.

Without this side-stepping logic mentioned above, even **without this patch** 
(so back to the "Ye Olde Strict Type Equality" mode), this particular function 
would be warned about, which is a definite false positive.


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

https://reviews.llvm.org/D75041

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

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Whisperity via Phabricator via cfe-commits]

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

After reading through the diff as rendered by Phabricateur, I realised I left 
in a few crappy debug prints and commented-out values that are not needed in 
the end. This is fixed now.


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

https://reviews.llvm.org/D75041

Files:
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.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-implicit-qualifiers.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.c
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.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-qualifiermixing.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
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
@@ -3,7 +3,8 @@
 // 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: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0} \
+// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0}, \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \
 // RUN:  ]}' -- -x c
 
 #define bool _Bool
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
@@ -3,7 +3,8 @@
 // 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: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 1} \
+// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 1}, \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \
 // RUN:  ]}' --
 
 typedef int MyInt1;
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.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-len3.cpp
@@ -3,7 +3,8 @@
 // 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: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0} \
+// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0}, \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \
 // RUN:  ]}' --
 
 int add(int Left, int Right) { return Left + Right; } // NO-WARN: Only 2 parameters.
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.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-len2.cpp
@@ -3,7 +3,8 @@
 // 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: {key: 

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

[Whisperity via Phabricator via cfe-commits]

whisperity updated this revision to Diff 327743.
whisperity retitled this revision from "[clang-tidy] Approximate implicit 
conversion issues in 
'experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type'" to 
"[clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability 
because of implicit conversions".
whisperity edited the summary of this revision.
whisperity added reviewers: hokein, alexfh, njames93.
whisperity set the repository for this revision to rG LLVM Github Monorepo.
whisperity added a comment.

- Rebased over updated, rewritten, newer base patch version, vastly increased 
the quality of the code.
- Tidied the tests and made them more self-explanatory.
- Implemented handling //function pointer conversion// (implicit losing of 
`noexcept`). This was missing from the previous uploaded patch.
- Ensured that implicit conversion sequences are diagnosed properly, even when 
typedefs are involved. This means that if the conversion takes 4-5-6 logical 
steps (e.g. instead of //`SomeFancyIntTypedef` -> `DoubleBoxingType`//, emit 
//`SomeFancyIntTypedef` -> `int` -> `double` -> `const double` -> 
`DoubleBoxingType` (user defined ctor)//) it's all explained properly.
- Respect the user's decision about the mixability of `T` and `const T` (the 
previous patch D96355 ) even in the context of 
implicit conversions (i.e. if the user did not allow that, do not consider a 
user-defined type which constructor takes a const mixable with a non-const 
other parameter).
- Put a diagnostic note to the location of the `FuntionDecl` of the conversion 
method involved in the mixability.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75041

Files:
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.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-implicit-qualifiers.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.c
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.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-qualifiermixing.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
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c
@@ -3,7 +3,8 @@
 // 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: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0} \
+// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0}, \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \
 // RUN:  ]}' -- -x c
 
 #define bool _Bool
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp
@@ -3,7 +3,8 @@
 // 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: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 1} \
+// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 1}, \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \
 // RUN:  ]}' --
 
 typedef int MyInt1;
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp
===
---