Index: clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tidy/misc/CMakeLists.txt +++ clang-tidy/misc/CMakeLists.txt @@ -5,6 +5,7 @@ BoolPointerImplicitConversion.cpp MiscTidyModule.cpp RedundantSmartptrGet.cpp + SwappedArgumentsCheck.cpp UseOverride.cpp LINK_LIBS Index: clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tidy/misc/MiscTidyModule.cpp +++ clang-tidy/misc/MiscTidyModule.cpp @@ -13,6 +13,7 @@ #include "ArgumentCommentCheck.h" #include "BoolPointerImplicitConversion.h" #include "RedundantSmartptrGet.h" +#include "SwappedArgumentsCheck.h" #include "UseOverride.h" namespace clang { @@ -31,6 +32,9 @@ "misc-redundant-smartptr-get", new ClangTidyCheckFactory()); CheckFactories.addCheckFactory( + "misc-swapped-arguments", + new ClangTidyCheckFactory()); + CheckFactories.addCheckFactory( "misc-use-override", new ClangTidyCheckFactory()); } Index: clang-tidy/misc/SwappedArgumentsCheck.h =================================================================== --- /dev/null +++ clang-tidy/misc/SwappedArgumentsCheck.h @@ -0,0 +1,30 @@ +//===--- SwappedArgumentsCheck.h - clang-tidy -------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SWAPPED_ARGUMENTS_CHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SWAPPED_ARGUMENTS_CHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { + +/// \brief Finds potentially swapped arguments by looking at implicit +/// conversions. +class SwappedArgumentsCheck : public ClangTidyCheck { +public: + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SWAPPED_ARGUMENTS_CHECK_H + Index: clang-tidy/misc/SwappedArgumentsCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/misc/SwappedArgumentsCheck.cpp @@ -0,0 +1,117 @@ +//===--- SwappedArgumentsCheck.cpp - clang-tidy ---------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "SwappedArgumentsCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/Lex/Lexer.h" +#include "llvm/ADT/SmallPtrSet.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { + +void SwappedArgumentsCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher(callExpr().bind("call"), this); +} + +/// \brief Look through lvalue to rvalue and nop casts. This filters out +/// implicit conversions that have no effect on the input but block our view for +/// other implicit casts. +static const Expr *ignoreNoOpCasts(const Expr *E) { + if (auto *Cast = dyn_cast(E)) + if (Cast->getCastKind() == CK_LValueToRValue || + Cast->getCastKind() == CK_NoOp) + return ignoreNoOpCasts(Cast->getSubExpr()); + return E; +} + +/// \brief Restrict the warning to implicit casts that are most likely +/// accidental. User defined or integral conversions fit in this category, +/// lvalue to rvalue or derived to base does not. +static bool isImplicitCastCandidate(const CastExpr *Cast) { + return Cast->getCastKind() == CK_UserDefinedConversion || + Cast->getCastKind() == CK_FloatingToBoolean || + Cast->getCastKind() == CK_FloatingToIntegral || + Cast->getCastKind() == CK_IntegralToBoolean || + Cast->getCastKind() == CK_IntegralToFloating || + Cast->getCastKind() == CK_MemberPointerToBoolean || + Cast->getCastKind() == CK_PointerToBoolean; +} + +/// \brief Get a StringRef representing a SourceRange. +static StringRef getAsString(const MatchFinder::MatchResult &Result, + SourceRange R) { + const SourceManager &SM = *Result.SourceManager; + // Don't even try to resolve macro or include contraptions. Not worth emitting + // a fixit for. + if (!SM.isWrittenInSameFile(R.getBegin(), R.getEnd())) + return StringRef(); + + const char *Begin = SM.getCharacterData(R.getBegin()); + const char *End = SM.getCharacterData(Lexer::getLocForEndOfToken( + R.getEnd(), 0, SM, Result.Context->getLangOpts())); + + return StringRef(Begin, End - Begin); +} + +void SwappedArgumentsCheck::check(const MatchFinder::MatchResult &Result) { + auto *Call = Result.Nodes.getStmtAs("call"); + + llvm::SmallPtrSet UsedArgs; + for (unsigned I = 1, E = Call->getNumArgs(); I < E; ++I) { + const Expr *LHS = Call->getArg(I - 1); + const Expr *RHS = Call->getArg(I); + + // Only need to check RHS, as LHS has already been covered. We don't want to + // emit two warnings for a single argument. + if (UsedArgs.count(RHS)) + continue; + + auto *LHSCast = dyn_cast(ignoreNoOpCasts(LHS)); + auto *RHSCast = dyn_cast(ignoreNoOpCasts(RHS)); + + // Look if this is a potentially swapped argument pair. First look for + // implicit casts. + if (!LHSCast || !RHSCast || !isImplicitCastCandidate(LHSCast) || + !isImplicitCastCandidate(RHSCast)) + continue; + + // If the types that go into the implicit casts match the types of the other + // argument in the declaration there is a high probability that the + // arguments were swapped. + // TODO: We could make use of the edit distance between the argument name + // and the name of the passed variable to get additional confidence. + if (LHS->getType() == RHS->getType() || + LHS->getType() != ignoreNoOpCasts(RHSCast->getSubExpr())->getType() || + RHS->getType() != ignoreNoOpCasts(LHSCast->getSubExpr())->getType()) + continue; + + // Emit a warning and fixits that swap the arguments. + auto D = diag(Call->getLocStart(), + "two matching implicit casts, potentially swapped arguments"); + + SourceRange LHSRange = LHS->getSourceRange(), + RHSRange = RHS->getSourceRange(); + StringRef RHSString = getAsString(Result, RHSRange); + StringRef LHSString = getAsString(Result, LHSRange); + if (!LHSString.empty() && !RHSString.empty()) { + D << FixItHint::CreateReplacement( + CharSourceRange::getTokenRange(LHSRange), RHSString) + << FixItHint::CreateReplacement( + CharSourceRange::getTokenRange(RHSRange), LHSString); + } + + // Remember that we emitted a warning for this argument. + UsedArgs.insert(RHSCast); + } +} + +} // namespace tidy +} // namespace clang Index: test/clang-tidy/misc-swapped-arguments.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-swapped-arguments.cpp @@ -0,0 +1,26 @@ +// RUN: $(dirname %s)/check_clang_tidy_fix.sh %s misc-swapped-arguments %t +// REQUIRES: shell + +void F(int, double); + +int SomeFunction(); + +void foo() { + F(1.0, 3); +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: two matching implicit casts, 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: two matching implicit casts, potentially swapped arguments +// In macro, don't emit fixits. +// CHECK-FIXES: F(b, M(Some, Function)); + + F(1.0, 1.0); // no-warning + F(3, 1.0); // no-warning + F(true, false); // no-warning + F(0, 'c'); // no-warning +} +