mclow.lists updated this revision to Diff 41662.
mclow.lists added a comment.
More tests; incorporated some of the suggestions for making sure we don't step
on other people's namespaces named `std`.
http://reviews.llvm.org/D15121
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/StdSwapCheck.cpp
clang-tidy/misc/StdSwapCheck.h
docs/clang-tidy/checks/list.rst
docs/clang-tidy/checks/misc-std-swap.rst
test/clang-tidy/misc-StdSwap.cpp
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 <typename T> 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);
+ }
+}
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: 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: 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<FileID, unsigned> 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<NestedNameSpecifierLoc>("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<CallExpr>("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: 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<SizeofContainerCheck>("misc-sizeof-container");
CheckFactories.registerCheck<StaticAssertCheck>(
"misc-static-assert");
+ CheckFactories.registerCheck<StdSwapCheck>(
+ "misc-std-swap");
CheckFactories.registerCheck<SwappedArgumentsCheck>(
"misc-swapped-arguments");
CheckFactories.registerCheck<ThrowByValueCatchByReferenceCheck>(
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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
