[clang-tools-extra] cc4f865 - [clang-tidy] Improve build-in type handling in bugprone-swapped-arguments
Piotr Zegar via cfe-commits
cfe-commits at lists.llvm.org
Sun Jul 16 11:56:44 PDT 2023
Author: Piotr Zegar
Date: 2023-07-16T18:29:29Z
New Revision: cc4f86562127f29560abb54d0c11f277b3373a2a
URL: https://github.com/llvm/llvm-project/commit/cc4f86562127f29560abb54d0c11f277b3373a2a
DIFF: https://github.com/llvm/llvm-project/commit/cc4f86562127f29560abb54d0c11f277b3373a2a.diff
LOG: [clang-tidy] Improve build-in type handling in bugprone-swapped-arguments
Improved detection of argument swaps involving integral and floating-point
types by enhancing handling of implicit conversions. Now implicit casts
from float to double are also considered, same for integers.
Improved documentation.
Fixes: #62926
Reviewed By: carlosgalvezp
Differential Revision: https://reviews.llvm.org/D151495
Added:
Modified:
clang-tools-extra/clang-tidy/bugprone/SwappedArgumentsCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone/swapped-arguments.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/swapped-arguments.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/bugprone/SwappedArgumentsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SwappedArgumentsCheck.cpp
index 078aed6911c5f0..8989444dde1300 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SwappedArgumentsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SwappedArgumentsCheck.cpp
@@ -17,7 +17,8 @@ using namespace clang::ast_matchers;
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 @@ static bool isImplicitCastCandidate(const CastExpr *Cast) {
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 @@ void SwappedArgumentsCheck::check(const MatchFinder::MatchResult &Result) {
// 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
index 7c6efe60a013ae..e2b2ea3491aa8f 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -313,6 +313,11 @@ Changes in existing checks
sequenced when constructor call is written as list-initialization. Understand
that there is a sequence point between designated initializers.
+- Improved :doc:`bugprone-swapped-arguments
+ <clang-tidy/checks/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
<clang-tidy/checks/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
index 30ae925da24a67..674108f9d01ed9 100644
--- 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
diff erent
+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
diff erent 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
index 06fb2d869f69bd..b2ad08be907a77 100644
--- 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 @@ template <typename T, typename U>
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 @@ void foo() {
#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);
}
More information about the cfe-commits
mailing list