Index: clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tidy/misc/CMakeLists.txt +++ clang-tidy/misc/CMakeLists.txt @@ -17,6 +17,7 @@ NonCopyableObjects.cpp SizeofContainerCheck.cpp StaticAssertCheck.cpp + StdSwapCheck.cpp SwappedArgumentsCheck.cpp ThrowByValueCatchByReferenceCheck.cpp UndelegatedConstructor.cpp Index: clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tidy/misc/MiscTidyModule.cpp +++ clang-tidy/misc/MiscTidyModule.cpp @@ -25,6 +25,7 @@ #include "NonCopyableObjects.h" #include "SizeofContainerCheck.h" #include "StaticAssertCheck.h" +#include "StdSwapCheck.h" #include "SwappedArgumentsCheck.h" #include "ThrowByValueCatchByReferenceCheck.h" #include "UndelegatedConstructor.h" @@ -68,6 +69,8 @@ CheckFactories.registerCheck("misc-sizeof-container"); CheckFactories.registerCheck( "misc-static-assert"); + CheckFactories.registerCheck( + "misc-std-swap"); CheckFactories.registerCheck( "misc-swapped-arguments"); CheckFactories.registerCheck( Index: clang-tidy/misc/StdSwapCheck.h =================================================================== --- clang-tidy/misc/StdSwapCheck.h +++ clang-tidy/misc/StdSwapCheck.h @@ -0,0 +1,35 @@ +//===--- StdSwapCheck.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_STD_SWAP_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STD_SWAP_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { + +/// A checker that finds ns-qualified calls to std::swap, and replaces them +/// with calls that use ADL instead. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/misc-std-swap.html +class StdSwapCheck : public ClangTidyCheck { +public: + StdSwapCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + 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_STD_SWAP_H + Index: clang-tidy/misc/StdSwapCheck.cpp =================================================================== --- clang-tidy/misc/StdSwapCheck.cpp +++ clang-tidy/misc/StdSwapCheck.cpp @@ -0,0 +1,100 @@ +//===--- StdSwapCheck.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 "StdSwapCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { + +/// \brief \arg Loc is the end of a statement range. This returns the location +/// of the semicolon following the statement. +/// If no semicolon is found or the location is inside a macro, the returned +/// source location will be invalid. +static SourceLocation findSemiAfterLocation(SourceLocation loc, + ASTContext &Ctx, + bool IsDecl) { + SourceManager &SM = Ctx.getSourceManager(); + if (loc.isMacroID()) { + if (!Lexer::isAtEndOfMacroExpansion(loc, SM, Ctx.getLangOpts(), &loc)) + return SourceLocation(); + } + loc = Lexer::getLocForEndOfToken(loc, /*Offset=*/0, SM, Ctx.getLangOpts()); + + // Break down the source location. + std::pair locInfo = SM.getDecomposedLoc(loc); + + // Try to load the file buffer. + bool invalidTemp = false; + StringRef file = SM.getBufferData(locInfo.first, &invalidTemp); + if (invalidTemp) + return SourceLocation(); + + const char *tokenBegin = file.data() + locInfo.second; + + // Lex from the start of the given location. + Lexer lexer(SM.getLocForStartOfFile(locInfo.first), + Ctx.getLangOpts(), + file.begin(), tokenBegin, file.end()); + Token tok; + lexer.LexFromRawLexer(tok); + if (tok.isNot(tok::semi)) { + if (!IsDecl) + return SourceLocation(); + // Declaration may be followed with other tokens; such as an __attribute, + // before ending with a semicolon. + return findSemiAfterLocation(tok.getLocation(), Ctx, /*IsDecl*/true); + } + + return tok.getLocation(); +} + + +void StdSwapCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( +// callExpr(callee(functionDecl(hasName("swap"))), + callExpr(callee(namedDecl(hasName("swap"))), + callee(expr(ignoringParenImpCasts(declRefExpr(has(nestedNameSpecifierLoc().bind("namespace"))))))).bind("swap"), + this); +} + +void StdSwapCheck::check(const MatchFinder::MatchResult &Result) { + const auto *nsNode = Result.Nodes.getNodeAs("namespace"); + const CharSourceRange nsSourceRange = CharSourceRange::getCharRange(nsNode->getSourceRange()); + const std::string nsStr = clang::Lexer::getSourceText(nsSourceRange, + *Result.SourceManager, Result.Context->getLangOpts()); + + bool isGlobalNS = NamespaceDecl::castToDeclContext(nsNode->getNestedNameSpecifier()->getAsNamespace())->getParent()->isTranslationUnit(); + if (isGlobalNS && (nsStr == "std" || nsStr == "::std")) { + const auto *swapNode = Result.Nodes.getNodeAs("swap"); + QualType argType = swapNode->getArg(0)->getType(); + + if (!argType->isAnyPointerType() && !argType->isBuiltinType()) { + auto Diag = diag(nsNode->getBeginLoc(), "let the compiler find the right swap via ADL"); + + const CharSourceRange swapSourceRange = CharSourceRange::getCharRange(swapNode->getSourceRange()); + const SourceLocation semiLoc = findSemiAfterLocation(swapSourceRange.getEnd(), *Result.Context, false); + assert(semiLoc != SourceLocation() && "Can't find the terminating semicolon"); + +// SourceRange replaceRange{swapNode->getSourceRange()}; + const SourceRange replaceRange{nsSourceRange.getBegin(), nsSourceRange.getEnd().getLocWithOffset(2)}; + std::string usingText = std::string("{ using ") + nsStr + "::swap; swap"; + Diag << FixItHint::CreateReplacement(replaceRange, usingText) + << FixItHint::CreateInsertion(semiLoc.getLocWithOffset(1), " }"); + } + } +} + +} // namespace tidy +} // namespace clang + Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -46,6 +46,7 @@ misc-non-copyable-objects misc-sizeof-container misc-static-assert + misc-std-swap misc-swapped-arguments misc-throw-by-value-catch-by-reference misc-undelegated-constructor Index: docs/clang-tidy/checks/misc-std-swap.rst =================================================================== --- docs/clang-tidy/checks/misc-std-swap.rst +++ docs/clang-tidy/checks/misc-std-swap.rst @@ -0,0 +1,19 @@ +misc-std-swap +============ + +Adding an overload for `std:swap` (in the `std` namespace) is explicitly forbidden by the standard. + +The best practice for implementing swap for user-defined data structures is to implement a non-member swap in the same namespace as the type. Then, when you wish to swap to values `x` and `y`, you call `swap(x,y)` without a namespace, and argument dependent lookup will find it. Unfortunately this will not work for types that have overloads of `swap` in namespace `std` (standard library types and primitive types). So you have to bring them into play with a `using` declaration. + +Instead of writing: +> std::swap(x,y); + +you should write: +> using std::swap; swap(x,y); + +This checker find this pattern and replaces it with the recommended usage; wrapping the call in a pair of braces to scope the using directive. For builtin types (such as `int` and `float`), as well as pointers, it leaves the calls to `std::swap` alone, because those are correct. + +FUTURE WORK: + +Find overloads of `swap` in namespace std and put them in the correct namespace. + Index: test/clang-tidy/misc-StdSwap.cpp =================================================================== --- test/clang-tidy/misc-StdSwap.cpp +++ test/clang-tidy/misc-StdSwap.cpp @@ -0,0 +1,94 @@ +// RUN: %check_clang_tidy %s misc-std-swap %t + +namespace std { + template void swap(T&, T&) {} + } + +// FIXME: Add something that triggers the check here. +// FIXME: Verify the applied fix. +// * Make the CHECK patterns specific enough and try to make verified lines +// unique to avoid incorrect matches. +// * Use {{}} for regular expressions. + +// Bad code; don't overload in namespace std +struct S1 { int x; }; +namespace std { void swap(S1& x, S1 &y) { swap(x.x, y.x); } }; + +// Swap in namespace with type +namespace foo { struct S2 { int i; }; void swap(S2& x, S2& y) {std::swap(x.i, y.i); } } + +// Swap in namespace. +namespace bar { + struct S3 { int i; }; + void swap(int&, int&) {} + namespace std { + void swap(S3& x, S3& y) { ::std::swap(x.i, y.i); } + } +} + +void test0() +{ + S1 i,j; + std::swap(i,j); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: let the compiler find the right swap via ADL + // CHECK-FIXES: { using std::swap; swap(i,j); } +} + +void test1(bool b) +{ + foo::S2 x,y; + if ( b ) + std::swap(x,y); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: let the compiler find the right swap via ADL + // CHECK-FIXES: { using std::swap; swap(x,y); } +} + +namespace baz { + void test2() + { + ::S1 i,j; + ::std::swap(i,j); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: let the compiler find the right swap via ADL + // CHECK-FIXES: { using ::std::swap; swap(i,j); } + } +} + +void test_neg0() // Swap two builtins +{ + { + int i,j; + std::swap(i,j); + } + { + float i,j; + std::swap(i,j); + } +} + +void test_neg1() // std::swap two pointers +{ + S1 *i, *j; + std::swap(i,j); +} + +void test_neg2() // Call a namespace-qualified swap that isn't "std::" +{ + { + int i,j; + bar::swap(i,j); + ::bar::swap(i,j); + } + { + bar::S3 i,j; + bar::std::swap(i,j); + ::bar::std::swap(i,j); + } +} + +namespace bar { + void test_neg3() // calling a non-global std::swap + { + S3 x,y; + std::swap(x,y); + } +}