whisperity updated this revision to Diff 354218.
whisperity added a comment.
**NC** Rebase & more buildbot shenanigans so we can get a pass!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78652/new/
https://reviews.llvm.org/D78652
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-relatedness.c
clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-relatedness.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
@@ -4,7 +4,8 @@
// 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.ModelImplicitConversions, value: 0} \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0}, \
+// RUN: {key: bugprone-easily-swappable-parameters.SuppressParametersUsedTogether, value: 0} \
// RUN: ]}' -- -x c
#define bool _Bool
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-relatedness.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-relatedness.cpp
@@ -0,0 +1,231 @@
+// 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: 0}, \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0}, \
+// RUN: {key: bugprone-easily-swappable-parameters.SuppressParametersUsedTogether, value: 1} \
+// RUN: ]}' --
+
+namespace std {
+template <typename T>
+T max(const T &A, const T &B);
+} // namespace std
+
+bool coin();
+void f(int);
+void g(int);
+void h(int, int);
+void i(int, bool);
+void i(int, char);
+
+struct Tmp {
+ int f(int);
+ int g(int, int);
+};
+
+struct Int {
+ int I;
+};
+
+void compare(int Left, int Right) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 2 adjacent parameters of 'compare' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-2]]:18: note: the first parameter in the range is 'Left'
+// CHECK-MESSAGES: :[[@LINE-3]]:28: note: the last parameter in the range is 'Right'
+
+int decideSequence(int A, int B) {
+ if (A)
+ return 1;
+ if (B)
+ return 2;
+ return 3;
+}
+// CHECK-MESSAGES: :[[@LINE-7]]:20: warning: 2 adjacent parameters of 'decideSequence' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-8]]:24: note: the first parameter in the range is 'A'
+// CHECK-MESSAGES: :[[@LINE-9]]:31: note: the last parameter in the range is 'B'
+
+int myMax(int A, int B) { // NO-WARN: Appears in same expression.
+ return A < B ? A : B;
+}
+
+int myMax2(int A, int B) { // NO-WARN: Appears in same expression.
+ if (A < B)
+ return A;
+ return B;
+}
+
+int myMax3(int A, int B) { // NO-WARN: Appears in same expression.
+ return std::max(A, B);
+}
+
+int binaryToUnary(int A, int) {
+ return A;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:19: warning: 2 adjacent parameters of 'binaryToUnary' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-4]]:23: note: the first parameter in the range is 'A'
+// CHECK-MESSAGES: :[[@LINE-5]]:29: note: the last parameter in the range is '<unnamed>'
+
+int randomReturn1(int A, int B) { // NO-WARN: Appears in same expression.
+ return coin() ? A : B;
+}
+
+int randomReturn2(int A, int B) { // NO-WARN: Both parameters returned.
+ if (coin())
+ return A;
+ return B;
+}
+
+int randomReturn3(int A, int B) { // NO-WARN: Both parameters returned.
+ bool Flip = coin();
+ if (Flip)
+ return A;
+ Flip = coin();
+ if (Flip)
+ return B;
+ Flip = coin();
+ if (!Flip)
+ return 0;
+ return -1;
+}
+
+void passthrough1(int A, int B) { // WARN: Different functions, different params.
+ f(A);
+ g(B);
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:19: warning: 2 adjacent parameters of 'passthrough1' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-5]]:23: note: the first parameter in the range is 'A'
+// CHECK-MESSAGES: :[[@LINE-6]]:30: note: the last parameter in the range is 'B'
+
+void passthrough2(int A, int B) { // NO-WARN: Passed to same index of same function.
+ f(A);
+ f(B);
+}
+
+void passthrough3(int A, int B) { // NO-WARN: Passed to same index of same funtion.
+ h(1, A);
+ h(1, B);
+}
+
+void passthrough4(int A, int B) { // WARN: Different index used.
+ h(1, A);
+ h(B, 2);
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:19: warning: 2 adjacent parameters of 'passthrough4' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-5]]:23: note: the first parameter in the range is 'A'
+// CHECK-MESSAGES: :[[@LINE-6]]:30: note: the last parameter in the range is 'B'
+
+void passthrough5(int A, int B) { // WARN: Different function overload.
+ i(A, false);
+ i(A, '\0');
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:19: warning: 2 adjacent parameters of 'passthrough5' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-5]]:23: note: the first parameter in the range is 'A'
+// CHECK-MESSAGES: :[[@LINE-6]]:30: note: the last parameter in the range is 'B'
+
+void passthrough6(int A, int B) { // NO-WARN: Passed to same index of same function.
+ Tmp Temp;
+ Temp.f(A);
+ Temp.f(B);
+}
+
+void passthrough7(int A, int B) { // NO-WARN: Passed to same index of same function.
+ // Clang-Tidy isn't path sensitive, the fact that the two objects we call the
+ // function on is different is not modelled.
+ Tmp Temp1, Temp2;
+ Temp1.f(A);
+ Temp2.f(B);
+}
+
+void passthrough8(int A, int B) { // WARN: Different functions used.
+ f(A);
+ Tmp{}.f(B);
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:19: warning: 2 adjacent parameters of 'passthrough8' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-5]]:23: note: the first parameter in the range is 'A'
+// CHECK-MESSAGES: :[[@LINE-6]]:30: note: the last parameter in the range is 'B'
+
+// Test that the matching of "passed-to-function" is done to the proper node.
+// Put simply, this test should not crash here.
+void forwardDeclared(int X);
+
+void passthrough9(int A, int B) { // NO-WARN: Passed to same index of same function.
+ forwardDeclared(A);
+ forwardDeclared(B);
+}
+
+void forwardDeclared(int X) {}
+
+void passthrough10(int A, int B) { // NO-WARN: Passed to same index of same function.
+ forwardDeclared(A);
+ forwardDeclared(B);
+}
+
+bool compare1(Int I, Int J) { // NO-WARN: Same member accessed.
+ int Val1 = I.I;
+ int Val2 = J.I;
+ return Val1 < Val2;
+}
+
+bool compare2(Tmp T1, Tmp T2) { // NO-WARN: Same member accessed.
+ int Val1 = T1.g(0, 1);
+ int Val2 = T2.g(2, 3);
+ return Val1 < Val2;
+}
+
+bool compare3(Tmp T1, Tmp T2) { // WARN: Different member accessed.
+ int Val1 = T1.f(0);
+ int Val2 = T2.g(1, 2);
+ return Val1 < Val2;
+}
+// CHECK-MESSAGES: :[[@LINE-5]]:15: warning: 2 adjacent parameters of 'compare3' of similar type ('Tmp')
+// CHECK-MESSAGES: :[[@LINE-6]]:19: note: the first parameter in the range is 'T1'
+// CHECK-MESSAGES: :[[@LINE-7]]:27: note: the last parameter in the range is 'T2'
+
+int rangeBreaker(int I, int J, int K, int L, int M, int N) {
+ // (I, J) swappable.
+
+ if (J == K) // (J, K) related.
+ return -1;
+
+ if (K + 2 > Tmp{}.f(K))
+ return M;
+
+ // (K, L, M) swappable.
+
+ return N; // (M, N) related.
+}
+// CHECK-MESSAGES: :[[@LINE-13]]:18: warning: 2 adjacent parameters of 'rangeBreaker' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-14]]:22: note: the first parameter in the range is 'I'
+// CHECK-MESSAGES: :[[@LINE-15]]:29: note: the last parameter in the range is 'J'
+// CHECK-MESSAGES: :[[@LINE-16]]:32: warning: 3 adjacent parameters of 'rangeBreaker' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-17]]:36: note: the first parameter in the range is 'K'
+// CHECK-MESSAGES: :[[@LINE-18]]:50: note: the last parameter in the range is 'M'
+
+int returnsNotOwnParameter(int I, int J, int K) {
+ const auto &Lambda = [&K](int L, int M, int N) {
+ if (K)
+ return L;
+ return M; // (L, M) related.
+ };
+
+ if (Lambda(-1, 0, 1))
+ return I;
+ return J; // (I, J) related.
+}
+// CHECK-MESSAGES: :[[@LINE-11]]:35: warning: 2 adjacent parameters of 'returnsNotOwnParameter' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-12]]:39: note: the first parameter in the range is 'J'
+// CHECK-MESSAGES: :[[@LINE-13]]:46: note: the last parameter in the range is 'K'
+// CHECK-MESSAGES: :[[@LINE-13]]:36: warning: 2 adjacent parameters of 'operator()' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-14]]:40: note: the first parameter in the range is 'M'
+// CHECK-MESSAGES: :[[@LINE-15]]:47: note: the last parameter in the range is 'N'
+
+int usedTogetherInCapture(int I, int J, int K) { // NO-WARN: Used together.
+ const auto &Lambda = [I, J, K]() {
+ int A = I + 1;
+ int B = J - 2;
+ int C = K * 3;
+ return A + B + C;
+ };
+ return Lambda();
+}
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-relatedness.c
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-relatedness.c
@@ -0,0 +1,30 @@
+// 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: 0}, \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0}, \
+// RUN: {key: bugprone-easily-swappable-parameters.SuppressParametersUsedTogether, value: 1} \
+// RUN: ]}' -- -x c
+
+int myprint();
+int add(int X, int Y);
+
+void notRelated(int A, int B) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: 2 adjacent parameters of 'notRelated' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-2]]:21: note: the first parameter in the range is 'A'
+// CHECK-MESSAGES: :[[@LINE-3]]:28: note: the last parameter in the range is 'B'
+
+int addedTogether(int A, int B) { return add(A, B); } // NO-WARN: Passed to same function.
+
+void passedToSameKNRFunction(int A, int B) {
+ myprint("foo", A);
+ myprint("bar", B);
+}
+// CHECK-MESSAGES: :[[@LINE-4]]:30: warning: 2 adjacent parameters of 'passedToSameKNRFunction' of similar type ('int')
+// CHECK-MESSAGES: :[[@LINE-5]]:34: note: the first parameter in the range is 'A'
+// CHECK-MESSAGES: :[[@LINE-6]]:41: note: the last parameter in the range is 'B'
+// This is actually a false positive: the "passed to same function" heuristic
+// can't map the parameter index 1 to A and B because myprint() has no
+// parameters.
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
@@ -4,7 +4,8 @@
// 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.ModelImplicitConversions, value: 0} \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0}, \
+// RUN: {key: bugprone-easily-swappable-parameters.SuppressParametersUsedTogether, 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
@@ -4,7 +4,8 @@
// 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.ModelImplicitConversions, value: 0} \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0}, \
+// RUN: {key: bugprone-easily-swappable-parameters.SuppressParametersUsedTogether, 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
@@ -4,7 +4,8 @@
// 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.ModelImplicitConversions, value: 0} \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0}, \
+// RUN: {key: bugprone-easily-swappable-parameters.SuppressParametersUsedTogether, value: 0} \
// RUN: ]}' --
namespace std {
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.cpp
@@ -4,7 +4,8 @@
// 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.ModelImplicitConversions, value: 1} \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 1}, \
+// RUN: {key: bugprone-easily-swappable-parameters.SuppressParametersUsedTogether, value: 0} \
// RUN: ]}' --
void implicitDoesntBreakOtherStuff(int A, int B) {}
Index: 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.c
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.c
@@ -4,7 +4,8 @@
// 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.ModelImplicitConversions, value: 1} \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 1}, \
+// RUN: {key: bugprone-easily-swappable-parameters.SuppressParametersUsedTogether, value: 0} \
// RUN: ]}' --
void implicitDoesntBreakOtherStuff(int A, int B) {}
Index: 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-implicit-qualifiers.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicit-qualifiers.cpp
@@ -4,7 +4,8 @@
// 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.ModelImplicitConversions, value: 1} \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 1}, \
+// RUN: {key: bugprone-easily-swappable-parameters.SuppressParametersUsedTogether, value: 0} \
// RUN: ]}' --
void numericAndQualifierConversion(int I, const double CD) { numericAndQualifierConversion(CD, I); }
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp
@@ -4,7 +4,8 @@
// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: "\"\";Foo;Bar"}, \
// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: "T"}, \
// RUN: {key: bugprone-easily-swappable-parameters.QualifiersMix, value: 0}, \
-// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0} \
+// RUN: {key: bugprone-easily-swappable-parameters.ModelImplicitConversions, value: 0}, \
+// RUN: {key: bugprone-easily-swappable-parameters.SuppressParametersUsedTogether, value: 0} \
// RUN: ]}' --
void ignoredUnnamed(int I, int, int) {} // NO-WARN: No >= 2 length of non-unnamed.
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst
@@ -140,6 +140,34 @@
`ConstIterator`, `const_reverse_iterator`, `ConstReverseIterator`.
In addition, `_Bool` (but not `_bool`) is also part of the default value.
+.. option:: SuppressParametersUsedTogether
+
+ Suppresses diagnostics about parameters that are used together or in a
+ similar fashion inside the function's body.
+ Defaults to `true`.
+ Specifying `false` will turn off the heuristics.
+
+ Currently, the following heuristics are implemented which will suppress the
+ warning about the parameter pair involved:
+
+ * The parameters are used in the same expression, e.g. ``f(a, b)`` or
+ ``a < b``.
+ * The parameters are further passed to the same function to the same
+ parameter of that function, of the same overload.
+ E.g. ``f(a, 1)`` and ``f(b, 2)`` to some ``f(T, int)``.
+
+ .. note::
+
+ The check does not perform path-sensitive analysis, and as such,
+ "same function" in this context means the same function declaration.
+ If the same member function of a type on two distinct instances are
+ called with the parameters, it will still be regarded as
+ "same function".
+
+ * The same member field is accessed, or member method is called of the
+ two parameters, e.g. ``a.foo()`` and ``b.foo()``.
+ * Separate ``return`` statements return either of the parameters on
+ different code paths.
Limitations
-----------
Index: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
+++ clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h
@@ -47,6 +47,10 @@
/// during analysis and consider types that are implicitly convertible to
/// one another mixable.
const bool ModelImplicitConversions;
+
+ /// If enabled, diagnostics for parameters that are used together in a
+ /// similar way are not emitted.
+ const bool SuppressParametersUsedTogether;
};
} // namespace bugprone
Index: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp
@@ -9,6 +9,7 @@
#include "EasilySwappableParametersCheck.h"
#include "../utils/OptionsUtils.h"
#include "clang/AST/ASTContext.h"
+#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Lex/Lexer.h"
#include "llvm/ADT/SmallSet.h"
@@ -65,6 +66,10 @@
/// The default value for the ModelImplicitConversions check option.
static constexpr bool DefaultModelImplicitConversions = true;
+/// The default value for suppressing diagnostics about parameters that are
+/// used together.
+static constexpr bool DefaultSuppressParametersUsedTogether = true;
+
using namespace clang::ast_matchers;
namespace clang {
@@ -74,7 +79,12 @@
using TheCheck = EasilySwappableParametersCheck;
namespace filter {
+class SimilarlyUsedParameterPairSuppressor;
+
static bool isIgnoredParameter(const TheCheck &Check, const ParmVarDecl *Node);
+static inline bool
+isSimilarlyUsedParameter(const SimilarlyUsedParameterPairSuppressor &Suppressor,
+ const ParmVarDecl *Param1, const ParmVarDecl *Param2);
} // namespace filter
namespace model {
@@ -1269,9 +1279,9 @@
return {MixFlags::None};
}
-static MixableParameterRange modelMixingRange(const TheCheck &Check,
- const FunctionDecl *FD,
- std::size_t StartIndex) {
+static MixableParameterRange modelMixingRange(
+ const TheCheck &Check, const FunctionDecl *FD, std::size_t StartIndex,
+ const filter::SimilarlyUsedParameterPairSuppressor &UsageBasedSuppressor) {
std::size_t NumParams = FD->getNumParams();
assert(StartIndex < NumParams && "out of bounds for start");
const ASTContext &Ctx = FD->getASTContext();
@@ -1297,6 +1307,19 @@
LLVM_DEBUG(llvm::dbgs()
<< "Check mix of #" << J << " against #" << I << "...\n");
+ if (isSimilarlyUsedParameter(UsageBasedSuppressor, Ith, Jth)) {
+ // Consider the two similarly used parameters to not be possible in a
+ // mix-up at the user's request, if they enabled this heuristic.
+ LLVM_DEBUG(llvm::dbgs() << "Parameters #" << I << " and #" << J
+ << " deemed related, ignoring...\n");
+
+ // If the parameter #I and #J mixes, then I is mixable with something
+ // in the current range, so the range has to be broken and I not
+ // included.
+ MixesOfIth.clear();
+ break;
+ }
+
Mix M{Jth, Ith,
calculateMixability(Check, Jth->getType(), Ith->getType(), Ctx,
Check.ModelImplicitConversions ? ICMM_All
@@ -1331,6 +1354,12 @@
} // namespace model
+/// Matches DeclRefExprs and their ignorable wrappers to ParmVarDecls.
+AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher<Stmt>, paramRefExpr) {
+ return expr(ignoringParenImpCasts(ignoringElidableConstructorCall(
+ declRefExpr(to(parmVarDecl().bind("param"))))));
+}
+
namespace filter {
/// Returns whether the parameter's name or the parameter's type's name is
@@ -1391,6 +1420,261 @@
return false;
}
+/// This namespace contains the implementations for the suppression of
+/// diagnostics from similaly used ("related") parameters.
+namespace relatedness_heuristic {
+
+static constexpr std::size_t SmallDataStructureSize = 4;
+
+template <typename T, std::size_t N = SmallDataStructureSize>
+using ParamToSmallSetMap =
+ llvm::DenseMap<const ParmVarDecl *, llvm::SmallSet<T, N>>;
+
+/// Returns whether the sets mapped to the two elements in the map have at
+/// least one element in common.
+template <typename MapTy, typename ElemTy>
+bool lazyMapOfSetsIntersectionExists(const MapTy &Map, const ElemTy &E1,
+ const ElemTy &E2) {
+ auto E1Iterator = Map.find(E1);
+ auto E2Iterator = Map.find(E2);
+ if (E1Iterator == Map.end() || E2Iterator == Map.end())
+ return false;
+
+ for (const auto &E1SetElem : E1Iterator->second)
+ if (llvm::find(E2Iterator->second, E1SetElem) != E2Iterator->second.end())
+ return true;
+
+ return false;
+}
+
+/// Implements the heuristic that marks two parameters related if there is
+/// a usage for both in the same strict expression subtree. A strict
+/// expression subtree is a tree which only includes Expr nodes, i.e. no
+/// Stmts and no Decls.
+class AppearsInSameExpr : public RecursiveASTVisitor<AppearsInSameExpr> {
+ using Base = RecursiveASTVisitor<AppearsInSameExpr>;
+
+ const FunctionDecl *FD;
+ const Expr *CurrentExprOnlyTreeRoot = nullptr;
+ llvm::DenseMap<const ParmVarDecl *,
+ llvm::SmallPtrSet<const Expr *, SmallDataStructureSize>>
+ ParentExprsForParamRefs;
+
+public:
+ void setup(const FunctionDecl *FD) {
+ this->FD = FD;
+ TraverseFunctionDecl(const_cast<FunctionDecl *>(FD));
+ }
+
+ bool operator()(const ParmVarDecl *Param1, const ParmVarDecl *Param2) const {
+ return lazyMapOfSetsIntersectionExists(ParentExprsForParamRefs, Param1,
+ Param2);
+ }
+
+ bool TraverseDecl(Decl *D) {
+ CurrentExprOnlyTreeRoot = nullptr;
+ return Base::TraverseDecl(D);
+ }
+
+ bool TraverseStmt(Stmt *S, DataRecursionQueue *Queue = nullptr) {
+ if (auto *E = dyn_cast_or_null<Expr>(S)) {
+ bool RootSetInCurrentStackFrame = false;
+ if (!CurrentExprOnlyTreeRoot) {
+ CurrentExprOnlyTreeRoot = E;
+ RootSetInCurrentStackFrame = true;
+ }
+
+ bool Ret = Base::TraverseStmt(S);
+
+ if (RootSetInCurrentStackFrame)
+ CurrentExprOnlyTreeRoot = nullptr;
+
+ return Ret;
+ }
+
+ // A Stmt breaks the strictly Expr subtree.
+ CurrentExprOnlyTreeRoot = nullptr;
+ return Base::TraverseStmt(S);
+ }
+
+ bool VisitDeclRefExpr(DeclRefExpr *DRE) {
+ if (!CurrentExprOnlyTreeRoot)
+ return true;
+
+ if (auto *PVD = dyn_cast<ParmVarDecl>(DRE->getDecl()))
+ if (llvm::find(FD->parameters(), PVD))
+ ParentExprsForParamRefs[PVD].insert(CurrentExprOnlyTreeRoot);
+
+ return true;
+ }
+};
+
+/// Implements the heuristic that marks two parameters related if there are
+/// two separate calls to the same function (overload) and the parameters are
+/// passed to the same index in both calls, i.e f(a, b) and f(a, c) passes
+/// b and c to the same index (2) of f(), marking them related.
+class PassedToSameFunction {
+ ParamToSmallSetMap<std::pair<const FunctionDecl *, unsigned>> TargetParams;
+
+public:
+ void setup(const FunctionDecl *FD) {
+ auto ParamsAsArgsInFnCalls =
+ match(functionDecl(forEachDescendant(
+ callExpr(forEachArgumentWithParam(
+ paramRefExpr(), parmVarDecl().bind("passed-to")))
+ .bind("call-expr"))),
+ *FD, FD->getASTContext());
+ for (const auto &Match : ParamsAsArgsInFnCalls) {
+ const auto *PassedParamOfThisFn = Match.getNodeAs<ParmVarDecl>("param");
+ const auto *CE = Match.getNodeAs<CallExpr>("call-expr");
+ const auto *PassedToParam = Match.getNodeAs<ParmVarDecl>("passed-to");
+ assert(PassedParamOfThisFn && CE && PassedToParam);
+
+ const FunctionDecl *CalledFn = CE->getDirectCallee();
+ if (!CalledFn)
+ continue;
+
+ llvm::Optional<unsigned> TargetIdx;
+ unsigned NumFnParams = CalledFn->getNumParams();
+ for (unsigned Idx = 0; Idx < NumFnParams; ++Idx)
+ if (CalledFn->getParamDecl(Idx) == PassedToParam)
+ TargetIdx.emplace(Idx);
+
+ assert(TargetIdx.hasValue() && "Matched, but didn't find index?");
+ TargetParams[PassedParamOfThisFn].insert(
+ {CalledFn->getCanonicalDecl(), *TargetIdx});
+ }
+ }
+
+ bool operator()(const ParmVarDecl *Param1, const ParmVarDecl *Param2) const {
+ return lazyMapOfSetsIntersectionExists(TargetParams, Param1, Param2);
+ }
+};
+
+/// Implements the heuristic that marks two parameters related if the same
+/// member is accessed (referred to) inside the current function's body.
+class AccessedSameMemberOf {
+ ParamToSmallSetMap<const Decl *> AccessedMembers;
+
+public:
+ void setup(const FunctionDecl *FD) {
+ auto MembersCalledOnParams = match(
+ functionDecl(forEachDescendant(
+ memberExpr(hasObjectExpression(paramRefExpr())).bind("mem-expr"))),
+ *FD, FD->getASTContext());
+
+ for (const auto &Match : MembersCalledOnParams) {
+ const auto *AccessedParam = Match.getNodeAs<ParmVarDecl>("param");
+ const auto *ME = Match.getNodeAs<MemberExpr>("mem-expr");
+ assert(AccessedParam && ME);
+ AccessedMembers[AccessedParam].insert(
+ ME->getMemberDecl()->getCanonicalDecl());
+ }
+ }
+
+ bool operator()(const ParmVarDecl *Param1, const ParmVarDecl *Param2) const {
+ return lazyMapOfSetsIntersectionExists(AccessedMembers, Param1, Param2);
+ }
+};
+
+/// Implements the heuristic that marks two parameters related if different
+/// ReturnStmts return them from the function.
+class Returned {
+ llvm::SmallVector<const ParmVarDecl *, SmallDataStructureSize> ReturnedParams;
+
+public:
+ void setup(const FunctionDecl *FD) {
+ // TODO: Handle co_return.
+ auto ParamReturns = match(functionDecl(forEachDescendant(
+ returnStmt(hasReturnValue(paramRefExpr())))),
+ *FD, FD->getASTContext());
+ for (const auto &Match : ParamReturns) {
+ const auto *ReturnedParam = Match.getNodeAs<ParmVarDecl>("param");
+ assert(ReturnedParam);
+
+ if (find(FD->parameters(), ReturnedParam) == FD->param_end())
+ // Inside the subtree of a FunctionDecl there might be ReturnStmts of
+ // a parameter that isn't the parameter of the function, e.g. in the
+ // case of lambdas.
+ continue;
+
+ ReturnedParams.emplace_back(ReturnedParam);
+ }
+ }
+
+ bool operator()(const ParmVarDecl *Param1, const ParmVarDecl *Param2) const {
+ return llvm::find(ReturnedParams, Param1) != ReturnedParams.end() &&
+ llvm::find(ReturnedParams, Param2) != ReturnedParams.end();
+ }
+};
+
+} // namespace relatedness_heuristic
+
+/// Helper class that is used to detect if two parameters of the same function
+/// are used in a similar fashion, to suppress the result.
+class SimilarlyUsedParameterPairSuppressor {
+ const bool Enabled;
+ relatedness_heuristic::AppearsInSameExpr SameExpr;
+ relatedness_heuristic::PassedToSameFunction PassToFun;
+ relatedness_heuristic::AccessedSameMemberOf SameMember;
+ relatedness_heuristic::Returned Returns;
+
+public:
+ SimilarlyUsedParameterPairSuppressor(const FunctionDecl *FD, bool Enable)
+ : Enabled(Enable) {
+ if (!Enable)
+ return;
+
+ SameExpr.setup(FD);
+ PassToFun.setup(FD);
+ SameMember.setup(FD);
+ Returns.setup(FD);
+ }
+
+ /// Returns whether the specified two parameters are deemed similarly used
+ /// or related by the heuristics.
+ bool operator()(const ParmVarDecl *Param1, const ParmVarDecl *Param2) const {
+ if (!Enabled)
+ return false;
+
+ LLVM_DEBUG(llvm::dbgs()
+ << "::: Matching similar usage / relatedness heuristic...\n");
+
+ if (SameExpr(Param1, Param2)) {
+ LLVM_DEBUG(llvm::dbgs() << "::: Used in the same expression.\n");
+ return true;
+ }
+
+ if (PassToFun(Param1, Param2)) {
+ LLVM_DEBUG(llvm::dbgs()
+ << "::: Passed to same function in different calls.\n");
+ return true;
+ }
+
+ if (SameMember(Param1, Param2)) {
+ LLVM_DEBUG(llvm::dbgs()
+ << "::: Same member field access or method called.\n");
+ return true;
+ }
+
+ if (Returns(Param1, Param2)) {
+ LLVM_DEBUG(llvm::dbgs() << "::: Both parameter returned.\n");
+ return true;
+ }
+
+ LLVM_DEBUG(llvm::dbgs() << "::: None.\n");
+ return false;
+ }
+};
+
+// (This function hoists the call to operator() of the wrapper, so we do not
+// need to define the previous class at the top of the file.)
+static inline bool
+isSimilarlyUsedParameter(const SimilarlyUsedParameterPairSuppressor &Suppressor,
+ const ParmVarDecl *Param1, const ParmVarDecl *Param2) {
+ return Suppressor(Param1, Param2);
+}
+
} // namespace filter
/// Matches functions that have at least the specified amount of parameters.
@@ -1604,7 +1888,10 @@
DefaultIgnoredParameterTypeSuffixes))),
QualifiersMix(Options.get("QualifiersMix", DefaultQualifiersMix)),
ModelImplicitConversions(Options.get("ModelImplicitConversions",
- DefaultModelImplicitConversions)) {}
+ DefaultModelImplicitConversions)),
+ SuppressParametersUsedTogether(
+ Options.get("SuppressParametersUsedTogether",
+ DefaultSuppressParametersUsedTogether)) {}
void EasilySwappableParametersCheck::storeOptions(
ClangTidyOptions::OptionMap &Opts) {
@@ -1615,6 +1902,8 @@
optutils::serializeStringList(IgnoredParameterTypeSuffixes));
Options.store(Opts, "QualifiersMix", QualifiersMix);
Options.store(Opts, "ModelImplicitConversions", ModelImplicitConversions);
+ Options.store(Opts, "SuppressParametersUsedTogether",
+ SuppressParametersUsedTogether);
}
void EasilySwappableParametersCheck::registerMatchers(MatchFinder *Finder) {
@@ -1647,6 +1936,11 @@
std::size_t NumParams = FD->getNumParams();
std::size_t MixableRangeStartIndex = 0;
+ // Spawn one suppressor and if the user requested, gather information from
+ // the AST for the parameters' usages.
+ filter::SimilarlyUsedParameterPairSuppressor UsageBasedSuppressor{
+ FD, SuppressParametersUsedTogether};
+
LLVM_DEBUG(llvm::dbgs() << "Begin analysis of " << getName(FD) << " with "
<< NumParams << " parameters...\n");
while (MixableRangeStartIndex < NumParams) {
@@ -1657,8 +1951,8 @@
continue;
}
- MixableParameterRange R =
- modelMixingRange(*this, FD, MixableRangeStartIndex);
+ MixableParameterRange R = modelMixingRange(
+ *this, FD, MixableRangeStartIndex, UsageBasedSuppressor);
assert(R.NumParamsChecked > 0 && "Ensure forward progress!");
MixableRangeStartIndex += R.NumParamsChecked;
if (R.NumParamsChecked < MinimumLength) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
