whisperity updated this revision to Diff 325036.
whisperity retitled this revision from "[clang-tidy] Add "ignore related
parameters" heuristics to
'experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type'" to
"[clang-tidy] Suppress reports to similarly used parameters in
'bugprone-easily-swappable-parameters'".
whisperity edited the summary of this revision.
whisperity added a comment.
- Rebased over updated, rewritten, newer base patch version.
- Added the heuristic for //"same member access"// to the patch (which was so
far only on my development branch).
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-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-relatedness.cpp
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,185 @@
+// RUN: %check_clang_tidy %s bugprone-easily-swappable-parameters %t \
+// RUN: -config='{CheckOptions: [ \
+// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 1}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""}, \
+// 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'
+
+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'
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
@@ -2,7 +2,8 @@
// RUN: -config='{CheckOptions: [ \
// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 3}, \
// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterNames, value: ""}, \
-// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""} \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""}, \
+// 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
@@ -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: ""} \
+// RUN: {key: bugprone-easily-swappable-parameters.IgnoredParameterTypeSuffixes, value: ""}, \
+// RUN: {key: bugprone-easily-swappable-parameters.SuppressParametersUsedTogether, value: 0} \
// RUN: ]}' --
#define assert(X) ((void)(X))
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
@@ -2,7 +2,8 @@
// RUN: -config='{CheckOptions: [ \
// RUN: {key: bugprone-easily-swappable-parameters.MinimumLength, value: 2}, \
// 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.IgnoredParameterTypeSuffixes, value: "T"}, \
+// 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
@@ -80,6 +80,34 @@
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``.
+ A value of ``false`` will turn off the heuristic.
+
+ Currently, the following heuristics are implemented, either of 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
@@ -38,6 +38,10 @@
/// The parameter typename suffixes (as written in the source code) to be
/// ignored.
const std::vector<std::string> IgnoredParameterTypeSuffixes;
+
+ /// If enabled, diagnostics for parameters that are used together in a
+ /// similar way will not be presented.
+ 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"
@@ -53,6 +54,10 @@
"reverse_iterator",
"reverse_const_iterator"});
+/// The default value for suppressing diagnostics about parameters that are
+/// used together.
+static constexpr bool DefaultSuppressParametersUsedTogether = true;
+
#ifndef NDEBUG
template <std::size_t Width>
@@ -92,7 +97,12 @@
using TheCheck = EasilySwappableParametersCheck;
namespace filter {
+class SimilarlyUsedParameterPairSuppressor;
+
static bool isIgnoredParameter(const TheCheck &Check, const ParmVarDecl *Node);
+static bool
+isSimilarlyUsedParameter(const SimilarlyUsedParameterPairSuppressor &Suppressor,
+ const ParmVarDecl *Param1, const ParmVarDecl *Param2);
} // namespace filter
namespace model {
@@ -209,9 +219,9 @@
return {MIX_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();
@@ -247,6 +257,19 @@
assert(M.flags() != MIX_Invalid && "All flags decayed!");
+ 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;
+ }
+
if (M.flags() != MIX_None)
MixesOfIth.emplace_back(std::move(M));
}
@@ -269,6 +292,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
@@ -329,6 +358,255 @@
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 *const FD;
+ const Expr *CurrentExprOnlyTreeRoot = nullptr;
+ llvm::DenseMap<const ParmVarDecl *,
+ llvm::SmallPtrSet<const Expr *, SmallDataStructureSize>>
+ ParentExprsForParamRefs;
+
+public:
+ explicit AppearsInSameExpr(const FunctionDecl *FD) : 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:
+ explicit PassedToSameFunction(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()->getCanonicalDecl();
+ 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, *TargetIdx});
+ }
+ }
+
+ bool operator()(const ParmVarDecl *Param1, const ParmVarDecl *Param2) const {
+ return lazyMapOfSetsIntersectionExists(TargetParams, Param1, Param2);
+ }
+};
+
+class AccessedSameMemberOf {
+ ParamToSmallSetMap<const Decl *> AccessedMembers;
+
+public:
+ explicit AccessedSameMemberOf(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);
+ }
+};
+
+class Returned {
+ llvm::SmallVector<const ParmVarDecl *, SmallDataStructureSize> ReturnedParams;
+
+public:
+ explicit Returned(const FunctionDecl *FD) {
+ 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())
+ // Returning an Expr to a ParmVarDecl that **isn't** parameter of the
+ // function should not be possible. Make sure a mistake of the matcher
+ // does **not** interfere with the heuristic's result.
+ 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;
+ llvm::Optional<relatedness_heuristic::AppearsInSameExpr> SameExpr;
+ llvm::Optional<relatedness_heuristic::PassedToSameFunction> PassToFun;
+ llvm::Optional<relatedness_heuristic::AccessedSameMemberOf> SameMember;
+ llvm::Optional<relatedness_heuristic::Returned> Returns;
+
+public:
+ SimilarlyUsedParameterPairSuppressor(const FunctionDecl *FD, bool Enable)
+ : Enabled(Enable) {
+ if (!Enable)
+ return;
+
+ SameExpr.emplace(FD);
+ PassToFun.emplace(FD);
+ SameMember.emplace(FD);
+ Returns.emplace(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 && (*SameExpr)(Param1, Param2)) {
+ LLVM_DEBUG(llvm::dbgs() << "::: Used in the same expression.\n");
+ return true;
+ }
+
+ if (PassToFun && (*PassToFun)(Param1, Param2)) {
+ LLVM_DEBUG(llvm::dbgs()
+ << "::: Passed to same function in different calls.\n");
+ return true;
+ }
+
+ if (SameMember && (*SameMember)(Param1, Param2)) {
+ LLVM_DEBUG(llvm::dbgs()
+ << "::: Same member field access or method called.\n");
+ return true;
+ }
+
+ if (Returns && (*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 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.
@@ -384,7 +662,10 @@
Options.get("IgnoredParameterNames", DefaultIgnoredParameterNames))),
IgnoredParameterTypeSuffixes(optutils::parseStringList(
Options.get("IgnoredParameterTypeSuffixes",
- DefaultIgnoredParameterTypeSuffixes))) {}
+ DefaultIgnoredParameterTypeSuffixes))),
+ SuppressParametersUsedTogether(
+ Options.get("SuppressParametersUsedTogether",
+ DefaultSuppressParametersUsedTogether)) {}
void EasilySwappableParametersCheck::storeOptions(
ClangTidyOptions::OptionMap &Opts) {
@@ -393,6 +674,8 @@
optutils::serializeStringList(IgnoredParameterNames));
Options.store(Opts, "IgnoredParameterTypeSuffixes",
optutils::serializeStringList(IgnoredParameterTypeSuffixes));
+ Options.store(Opts, "SuppressParametersUsedTogether",
+ SuppressParametersUsedTogether);
}
void EasilySwappableParametersCheck::registerMatchers(MatchFinder *Finder) {
@@ -422,6 +705,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) {
@@ -433,8 +721,8 @@
continue;
}
- model::MixableParameterRange R =
- model::modelMixingRange(*this, FD, MixableRangeStartIndex);
+ model::MixableParameterRange R = model::modelMixingRange(
+ *this, FD, MixableRangeStartIndex, UsageBasedSuppressor);
assert(R.NumParamsChecked > 0 && "Ensure forward progress!");
MixableRangeStartIndex += R.NumParamsChecked;
if (R.NumParamsChecked < MinimumLength) {
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
