diff --git a/clang-tools-extra/clang-tidy/bugprone/SwappedArgumentsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SwappedArgumentsCheck.cpp --- a/clang-tools-extra/clang-tidy/bugprone/SwappedArgumentsCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/SwappedArgumentsCheck.cpp @@ -17,7 +17,8 @@ namespace clang::tidy::bugprone { void SwappedArgumentsCheck::registerMatchers(MatchFinder *Finder) { - Finder->addMatcher(callExpr().bind("call"), this); + Finder->addMatcher(callExpr(unless(isInTemplateInstantiation())).bind("call"), + this); } /// Look through lvalue to rvalue and nop casts. This filters out @@ -41,7 +42,42 @@ Cast->getCastKind() == CK_IntegralToBoolean || Cast->getCastKind() == CK_IntegralToFloating || Cast->getCastKind() == CK_MemberPointerToBoolean || - Cast->getCastKind() == CK_PointerToBoolean; + Cast->getCastKind() == CK_PointerToBoolean || + (Cast->getCastKind() == CK_IntegralCast && + Cast->getSubExpr()->getType()->isBooleanType()); +} + +static bool areTypesSemiEqual(const QualType L, const QualType R) { + if (L == R) + return true; + + if (!L->isBuiltinType() || !R->isBuiltinType()) + return false; + + return (L->isFloatingType() && R->isFloatingType()) || + (L->isIntegerType() && R->isIntegerType()) || + (L->isBooleanType() && R->isBooleanType()); +} + +static bool areArgumentsPotentiallySwapped(const QualType LTo, + const QualType RTo, + const QualType LFrom, + const QualType RFrom) { + if (LTo == RTo || LFrom == RFrom) + return false; + + const bool REq = areTypesSemiEqual(RTo, LFrom); + if (LTo == RFrom && REq) + return true; + + bool LEq = areTypesSemiEqual(LTo, RFrom); + if (RTo == LFrom && LEq) + return true; + + if (REq && LEq && !areTypesSemiEqual(RTo, LTo)) + return true; + + return false; } void SwappedArgumentsCheck::check(const MatchFinder::MatchResult &Result) { @@ -75,18 +111,16 @@ // heuristic. const Expr *LHSFrom = ignoreNoOpCasts(LHSCast->getSubExpr()); const Expr *RHSFrom = ignoreNoOpCasts(RHSCast->getSubExpr()); - if (LHS->getType() == RHS->getType() || - LHS->getType() != RHSFrom->getType() || - RHS->getType() != LHSFrom->getType()) + if (!areArgumentsPotentiallySwapped(LHS->getType(), RHS->getType(), + LHSFrom->getType(), RHSFrom->getType())) continue; // Emit a warning and fix-its that swap the arguments. diag(Call->getBeginLoc(), "argument with implicit conversion from %0 " "to %1 followed by argument converted from " "%2 to %3, potentially swapped arguments.") - << LHS->getType() << LHSFrom->getType() << RHS->getType() - << RHSFrom->getType() - << tooling::fixit::createReplacement(*LHS, *RHS, Ctx) + << LHSFrom->getType() << LHS->getType() << RHSFrom->getType() + << RHS->getType() << tooling::fixit::createReplacement(*LHS, *RHS, Ctx) << tooling::fixit::createReplacement(*RHS, *LHS, Ctx); // Remember that we emitted a warning for this argument. diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -313,6 +313,11 @@ sequenced when constructor call is written as list-initialization. Understand that there is a sequence point between designated initializers. +- Improved :doc:`bugprone-swapped-arguments + ` by enhancing handling of + implicit conversions, resulting in better detection of argument swaps + involving integral and floating-point types. + - Deprecated :doc:`cert-dcl21-cpp ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/swapped-arguments.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/swapped-arguments.rst --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/swapped-arguments.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/swapped-arguments.rst @@ -3,4 +3,39 @@ bugprone-swapped-arguments ========================== -Finds potentially swapped arguments by looking at implicit conversions. +Finds potentially swapped arguments by examining implicit conversions. +It analyzes the types of the arguments being passed to a function and compares +them to the expected types of the corresponding parameters. If there is a +mismatch or an implicit conversion that indicates a potential swap, a warning +is raised. + +.. code-block:: c++ + + void printNumbers(int a, float b); + + int main() { + // Swapped arguments: float passed as int, int as float) + printNumbers(10.0f, 5); + return 0; + } + +Covers a wide range of implicit conversions, including: +- User-defined conversions +- Conversions from floating-point types to boolean or integral types +- Conversions from integral types to boolean or floating-point types +- Conversions from boolean to integer types or floating-point types +- Conversions from (member) pointers to boolean + +It is important to note that for most argument swaps, the types need to match +exactly. However, there are exceptions to this rule. Specifically, when the +swapped argument is of integral type, an exact match is not always necessary. +Implicit casts from other integral types are also accepted. Similarly, when +dealing with floating-point arguments, implicit casts between different +floating-point types are considered acceptable. + +To avoid confusion, swaps where both swapped arguments are of integral types or +both are of floating-point types do not trigger the warning. In such cases, it's +assumed that the developer intentionally used different integral or +floating-point types and does not raise a warning. This approach prevents false +positives and provides flexibility in handling situations where varying integral +or floating-point types are intentionally utilized. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/swapped-arguments.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/swapped-arguments.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/swapped-arguments.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/swapped-arguments.cpp @@ -8,26 +8,31 @@ void G(T a, U b) { F(a, b); // no-warning F(2.0, 4); -// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'int' to 'double' followed by argument converted from 'double' to 'int', potentially swapped arguments. +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'double' to 'int' followed by argument converted from 'int' to 'double', potentially swapped arguments. // CHECK-FIXES: F(4, 2.0) } +void funShortFloat(short, float); +void funFloatFloat(float, float); +void funBoolShort(bool, short); +void funBoolFloat(bool, float); + void foo() { F(1.0, 3); -// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'int' to 'double' followed by argument converted from 'double' to 'int', potentially swapped arguments. +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'double' to 'int' followed by argument converted from 'int' to 'double', potentially swapped arguments. // CHECK-FIXES: F(3, 1.0) #define M(x, y) x##y() double b = 1.0; F(b, M(Some, Function)); -// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'int' to 'double' followed by argument converted from 'double' to 'int', potentially swapped arguments. +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'double' to 'int' followed by argument converted from 'int' to 'double', potentially swapped arguments. // CHECK-FIXES: F(M(Some, Function), b); #define N F(b, SomeFunction()) N; -// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'int' to 'double' followed by argument converted from 'double' to 'int', potentially swapped arguments. +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'double' to 'int' followed by argument converted from 'int' to 'double', potentially swapped arguments. // In macro, don't emit fixits. // CHECK-FIXES: #define N F(b, SomeFunction()) @@ -42,12 +47,32 @@ #define APPLY(f, x, y) f(x, y) APPLY(F, 1.0, 3); -// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: argument with implicit conversion from 'int' to 'double' followed by argument converted from 'double' to 'int', potentially swapped arguments. +// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: argument with implicit conversion from 'double' to 'int' followed by argument converted from 'int' to 'double', potentially swapped arguments. // CHECK-FIXES: APPLY(F, 3, 1.0); #define PARAMS 1.0, 3 #define CALL(P) F(P) CALL(PARAMS); -// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'int' to 'double' followed by argument converted from 'double' to 'int', potentially swapped arguments. +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'double' to 'int' followed by argument converted from 'int' to 'double', potentially swapped arguments. // In macro, don't emit fixits. + + funShortFloat(5.0, 1U); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'double' to 'short' followed by argument converted from 'unsigned int' to 'float', potentially swapped arguments. + // CHECK-FIXES: funShortFloat(1U, 5.0); + funShortFloat(5.0, 1); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'double' to 'short' followed by argument converted from 'int' to 'float', potentially swapped arguments. + // CHECK-FIXES: funShortFloat(1, 5.0); + funShortFloat(5.0f, 1); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'float' to 'short' followed by argument converted from 'int' to 'float', potentially swapped arguments. + // CHECK-FIXES: funShortFloat(1, 5.0f); + + funFloatFloat(1.0f, 2.0); + + funBoolShort(1U, true); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'unsigned int' to 'bool' followed by argument converted from 'bool' to 'short', potentially swapped arguments. + // CHECK-FIXES: funBoolShort(true, 1U); + + funBoolFloat(1.0, true); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: argument with implicit conversion from 'double' to 'bool' followed by argument converted from 'bool' to 'float', potentially swapped arguments. + // CHECK-FIXES: funBoolFloat(true, 1.0); }