mclow.lists updated the summary for this revision.
mclow.lists updated this revision to Diff 41559.
mclow.lists added a comment.
Suggestions from @AaronBallman; now does the substitution correctly, and passes
the tests.
Note: The diagnostics that clang emits are still whacko.
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-StdSwap.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,62 @@
+// RUN: %check_clang_tidy %s misc-std-swap %t
+
+#include <utility>
+
+// 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 { void swap(int, int) {} }
+
+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); }
+}
+
+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);
+}
+
Index: docs/clang-tidy/checks/misc-StdSwap.rst
===================================================================
--- docs/clang-tidy/checks/misc-StdSwap.rst
+++ docs/clang-tidy/checks/misc-StdSwap.rst
@@ -0,0 +1,19 @@
+misc-StdSwap
+============
+
+Adding an overload for `std:swap` (in the `std` namespace) is explicitly forbidden by the standard.
+
+The best practices 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 ADL 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" statement.
+
+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 brackets 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
@@ -32,6 +32,7 @@
llvm-include-order
llvm-namespace-comment
llvm-twine-local
+ misc-StdSwap
misc-argument-comment
misc-assert-side-effect
misc-assign-operator-signature
@@ -46,6 +47,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,34 @@
+//===--- 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 {
+
+/// FIXME: Write a short description.
+///
+/// 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,97 @@
+//===--- 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");
+ auto nsSourceRange = CharSourceRange::getCharRange(nsNode->getSourceRange());
+ const std::string nsStr = clang::Lexer::getSourceText(nsSourceRange,
+ *Result.SourceManager, Result.Context->getLangOpts());
+
+ if (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 auto swapSourceRange = CharSourceRange::getCharRange(swapNode->getSourceRange());
+ SourceLocation semiLoc = findSemiAfterLocation(swapSourceRange.getEnd(), *Result.Context, false);
+ assert(semiLoc != SourceLocation());
+
+ nsSourceRange.setEnd(nsSourceRange.getEnd().getLocWithOffset(2));
+ Diag << FixItHint::CreateReplacement(nsSourceRange, "{ using std::swap; ")
+ << 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
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
