Skip to content

Commit 082bf7f

Browse files
d0kd0k
committedJul 14, 2014
[clang-tidy] Add a checker for swapped arguments.
This looks for swapped arguments by looking at implicit conversions of arguments void Foo(int, double); Foo(1.0, 3); // Most likely a bug llvm-svn: 212942
1 parent ad21688 commit 082bf7f

File tree

5 files changed

+204
-0
lines changed

5 files changed

+204
-0
lines changed
 

‎clang-tools-extra/clang-tidy/misc/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ add_clang_library(clangTidyMiscModule
55
BoolPointerImplicitConversion.cpp
66
MiscTidyModule.cpp
77
RedundantSmartptrGet.cpp
8+
SwappedArgumentsCheck.cpp
89
UseOverride.cpp
910

1011
LINK_LIBS

‎clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
#include "ArgumentCommentCheck.h"
1414
#include "BoolPointerImplicitConversion.h"
1515
#include "RedundantSmartptrGet.h"
16+
#include "SwappedArgumentsCheck.h"
1617
#include "UseOverride.h"
1718

1819
namespace clang {
@@ -30,6 +31,9 @@ class MiscModule : public ClangTidyModule {
3031
CheckFactories.addCheckFactory(
3132
"misc-redundant-smartptr-get",
3233
new ClangTidyCheckFactory<RedundantSmartptrGet>());
34+
CheckFactories.addCheckFactory(
35+
"misc-swapped-arguments",
36+
new ClangTidyCheckFactory<SwappedArgumentsCheck>());
3337
CheckFactories.addCheckFactory(
3438
"misc-use-override",
3539
new ClangTidyCheckFactory<UseOverride>());

‎clang-tools-extra/clang-tidy/misc/SwappedArgumentsCheck.cpp

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
//===--- SwappedArgumentsCheck.cpp - clang-tidy ---------------------------===//
2+
//
3+
// The LLVM Compiler Infrastructure
4+
//
5+
// This file is distributed under the University of Illinois Open Source
6+
// License. See LICENSE.TXT for details.
7+
//
8+
//===----------------------------------------------------------------------===//
9+
10+
#include "SwappedArgumentsCheck.h"
11+
#include "clang/AST/ASTContext.h"
12+
#include "clang/Lex/Lexer.h"
13+
#include "llvm/ADT/SmallPtrSet.h"
14+
15+
using namespace clang::ast_matchers;
16+
17+
namespace clang {
18+
namespace tidy {
19+
20+
void SwappedArgumentsCheck::registerMatchers(MatchFinder *Finder) {
21+
Finder->addMatcher(callExpr().bind("call"), this);
22+
}
23+
24+
/// \brief Look through lvalue to rvalue and nop casts. This filters out
25+
/// implicit conversions that have no effect on the input but block our view for
26+
/// other implicit casts.
27+
static const Expr *ignoreNoOpCasts(const Expr *E) {
28+
if (auto *Cast = dyn_cast<CastExpr>(E))
29+
if (Cast->getCastKind() == CK_LValueToRValue ||
30+
Cast->getCastKind() == CK_NoOp)
31+
return ignoreNoOpCasts(Cast->getSubExpr());
32+
return E;
33+
}
34+
35+
/// \brief Restrict the warning to implicit casts that are most likely
36+
/// accidental. User defined or integral conversions fit in this category,
37+
/// lvalue to rvalue or derived to base does not.
38+
static bool isImplicitCastCandidate(const CastExpr *Cast) {
39+
return Cast->getCastKind() == CK_UserDefinedConversion ||
40+
Cast->getCastKind() == CK_FloatingToBoolean ||
41+
Cast->getCastKind() == CK_FloatingToIntegral ||
42+
Cast->getCastKind() == CK_IntegralToBoolean ||
43+
Cast->getCastKind() == CK_IntegralToFloating ||
44+
Cast->getCastKind() == CK_MemberPointerToBoolean ||
45+
Cast->getCastKind() == CK_PointerToBoolean;
46+
}
47+
48+
/// \brief Get a StringRef representing a SourceRange.
49+
static StringRef getAsString(const MatchFinder::MatchResult &Result,
50+
SourceRange R) {
51+
const SourceManager &SM = *Result.SourceManager;
52+
// Don't even try to resolve macro or include contraptions. Not worth emitting
53+
// a fixit for.
54+
if (R.getBegin().isMacroID() ||
55+
!SM.isWrittenInSameFile(R.getBegin(), R.getEnd()))
56+
return StringRef();
57+
58+
const char *Begin = SM.getCharacterData(R.getBegin());
59+
const char *End = SM.getCharacterData(Lexer::getLocForEndOfToken(
60+
R.getEnd(), 0, SM, Result.Context->getLangOpts()));
61+
62+
return StringRef(Begin, End - Begin);
63+
}
64+
65+
void SwappedArgumentsCheck::check(const MatchFinder::MatchResult &Result) {
66+
auto *Call = Result.Nodes.getStmtAs<CallExpr>("call");
67+
68+
llvm::SmallPtrSet<const Expr *, 4> UsedArgs;
69+
for (unsigned I = 1, E = Call->getNumArgs(); I < E; ++I) {
70+
const Expr *LHS = Call->getArg(I - 1);
71+
const Expr *RHS = Call->getArg(I);
72+
73+
// Only need to check RHS, as LHS has already been covered. We don't want to
74+
// emit two warnings for a single argument.
75+
if (UsedArgs.count(RHS))
76+
continue;
77+
78+
const auto *LHSCast = dyn_cast<ImplicitCastExpr>(ignoreNoOpCasts(LHS));
79+
const auto *RHSCast = dyn_cast<ImplicitCastExpr>(ignoreNoOpCasts(RHS));
80+
81+
// Look if this is a potentially swapped argument pair. First look for
82+
// implicit casts.
83+
if (!LHSCast || !RHSCast || !isImplicitCastCandidate(LHSCast) ||
84+
!isImplicitCastCandidate(RHSCast))
85+
continue;
86+
87+
// If the types that go into the implicit casts match the types of the other
88+
// argument in the declaration there is a high probability that the
89+
// arguments were swapped.
90+
// TODO: We could make use of the edit distance between the argument name
91+
// and the name of the passed variable in addition to this type based
92+
// heuristic.
93+
const Expr *LHSFrom = ignoreNoOpCasts(LHSCast->getSubExpr());
94+
const Expr *RHSFrom = ignoreNoOpCasts(RHSCast->getSubExpr());
95+
if (LHS->getType() == RHS->getType() ||
96+
LHS->getType() != RHSFrom->getType() ||
97+
RHS->getType() != LHSFrom->getType())
98+
continue;
99+
100+
// Emit a warning and fix-its that swap the arguments.
101+
SourceRange LHSRange = LHS->getSourceRange(),
102+
RHSRange = RHS->getSourceRange();
103+
auto D =
104+
diag(Call->getLocStart(), "argument with implicit conversion from %0 "
105+
"to %1 followed by argument converted from "
106+
"%2 to %3, potentially swapped arguments.")
107+
<< LHS->getType() << LHSFrom->getType() << RHS->getType()
108+
<< RHSFrom->getType() << LHSRange << RHSRange;
109+
110+
StringRef RHSString = getAsString(Result, RHSRange);
111+
StringRef LHSString = getAsString(Result, LHSRange);
112+
if (!LHSString.empty() && !RHSString.empty()) {
113+
D << FixItHint::CreateReplacement(
114+
CharSourceRange::getTokenRange(LHSRange), RHSString)
115+
<< FixItHint::CreateReplacement(
116+
CharSourceRange::getTokenRange(RHSRange), LHSString);
117+
}
118+
119+
// Remember that we emitted a warning for this argument.
120+
UsedArgs.insert(RHSCast);
121+
}
122+
}
123+
124+
} // namespace tidy
125+
} // namespace clang

‎clang-tools-extra/clang-tidy/misc/SwappedArgumentsCheck.h

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
//===--- SwappedArgumentsCheck.h - clang-tidy -------------------*- C++ -*-===//
2+
//
3+
// The LLVM Compiler Infrastructure
4+
//
5+
// This file is distributed under the University of Illinois Open Source
6+
// License. See LICENSE.TXT for details.
7+
//
8+
//===----------------------------------------------------------------------===//
9+
10+
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SWAPPED_ARGUMENTS_CHECK_H
11+
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SWAPPED_ARGUMENTS_CHECK_H
12+
13+
#include "../ClangTidy.h"
14+
15+
namespace clang {
16+
namespace tidy {
17+
18+
/// \brief Finds potentially swapped arguments by looking at implicit
19+
/// conversions.
20+
class SwappedArgumentsCheck : public ClangTidyCheck {
21+
public:
22+
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
23+
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
24+
};
25+
26+
} // namespace tidy
27+
} // namespace clang
28+
29+
#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SWAPPED_ARGUMENTS_CHECK_H
30+

‎clang-tools-extra/test/clang-tidy/misc-swapped-arguments.cpp

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
// RUN: $(dirname %s)/check_clang_tidy_fix.sh %s misc-swapped-arguments %t
2+
// REQUIRES: shell
3+
4+
void F(int, double);
5+
6+
int SomeFunction();
7+
8+
template <typename T, typename U>
9+
void G(T a, U b) {
10+
F(a, b); // no-warning
11+
F(2.0, 4);
12+
// 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.
13+
// CHECK-FIXES: F(4, 2.0)
14+
}
15+
16+
void foo() {
17+
F(1.0, 3);
18+
// 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.
19+
// CHECK-FIXES: F(3, 1.0)
20+
21+
#define M(x, y) x##y()
22+
23+
double b = 1.0;
24+
F(b, M(Some, Function));
25+
// 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.
26+
// In macro, don't emit fixits.
27+
// CHECK-FIXES: F(b, M(Some, Function));
28+
29+
#define N F(b, SomeFunction())
30+
31+
N;
32+
// 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.
33+
// In macro, don't emit fixits.
34+
// CHECK-FIXES: #define N F(b, SomeFunction())
35+
36+
G(b, 3);
37+
G(3, 1.0);
38+
G(0, 0);
39+
40+
F(1.0, 1.0); // no-warning
41+
F(3, 1.0); // no-warning
42+
F(true, false); // no-warning
43+
F(0, 'c'); // no-warning
44+
}

0 commit comments

Comments
 (0)
Please sign in to comment.