diff --git a/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h b/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h --- a/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.h +++ b/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 diff --git a/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp --- a/clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp +++ b/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, 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 +using ParamToSmallSetMap = + llvm::DenseMap>; + +/// Returns whether the sets mapped to the two elements in the map have at +/// least one element in common. +template +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 { + using Base = RecursiveASTVisitor; + + const FunctionDecl *FD; + const Expr *CurrentExprOnlyTreeRoot = nullptr; + llvm::DenseMap> + ParentExprsForParamRefs; + +public: + void setup(const FunctionDecl *FD) { + this->FD = FD; + TraverseFunctionDecl(const_cast(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(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(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> 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("param"); + const auto *CE = Match.getNodeAs("call-expr"); + const auto *PassedToParam = Match.getNodeAs("passed-to"); + assert(PassedParamOfThisFn && CE && PassedToParam); + + const FunctionDecl *CalledFn = CE->getDirectCallee(); + if (!CalledFn) + continue; + + llvm::Optional 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 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("param"); + const auto *ME = Match.getNodeAs("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 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("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) { diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone-easily-swappable-parameters.rst +++ b/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 ----------- diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-ignore.cpp +++ b/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. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicit-qualifiers.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicit-qualifiers.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicit-qualifiers.cpp +++ b/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); } diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.c --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.c +++ b/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) {} diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-implicits.cpp +++ b/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) {} diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len2.cpp +++ b/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 { diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-len3.cpp +++ b/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. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-qualifiermixing.cpp +++ b/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; diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-relatedness.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-relatedness.c new file mode 100644 --- /dev/null +++ b/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. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-relatedness.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters-relatedness.cpp new file mode 100644 --- /dev/null +++ b/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 +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 '' + +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(); +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-easily-swappable-parameters.c +++ b/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