[PATCH] D15121: A new clang-tidy module to find calls to `std::swap`, and change them to use ADL

[Richard Smith via cfe-commits]

rsmith added a comment.

In http://reviews.llvm.org/D15121#300033, @LegalizeAdulthood wrote:

> I'm wondering if there isn't an existing module that would be a good fit for this check.  Why not in the modernize module?


This isn't really modernization, it's a bug fix. ADL has always been the right way to find `swap`.


================
Comment at: clang-tidy/misc/StdSwapCheck.cpp:24
@@ +23,3 @@
+/// source location will be invalid.
+static SourceLocation findSemiAfterLocation(SourceLocation loc,
+                                            ASTContext &Ctx,
----------------
Is there somewhere more central where this can live?

================
Comment at: clang-tidy/misc/StdSwapCheck.cpp:67
@@ +66,3 @@
+    callExpr(callee(namedDecl(hasName("swap"))),
+    callee(expr(ignoringParenImpCasts(declRefExpr(has(nestedNameSpecifierLoc().bind("namespace"))))))).bind("swap"),
+    this);
----------------
I think the two `callee` checks can be folded into one. Can you put the check that the namespace name is "std" in here too?

================
Comment at: clang-tidy/misc/StdSwapCheck.cpp:67
@@ +66,3 @@
+    callExpr(callee(namedDecl(hasName("swap"))),
+    callee(expr(ignoringParenImpCasts(declRefExpr(has(nestedNameSpecifierLoc().bind("namespace"))))))).bind("swap"),
+    this);
----------------
rsmith wrote:
> I think the two `callee` checks can be folded into one. Can you put the check that the namespace name is "std" in here too?
Ignoring parens here will lead to your fixit not working. Instead of trying to replace the "std::" with "{ using std::swap; ", maybe it'd be better and safer to replace the entire callee with "{ using std::swap; swap"?

================
Comment at: clang-tidy/misc/StdSwapCheck.cpp:77
@@ +76,3 @@
+
+  if (nsStr == "std") {
+	const auto *swapNode = Result.Nodes.getNodeAs<CallExpr>("swap");
----------------
Presumably this should also be checking that the parent of `nsNode` is the translation unit; we're not looking for a `std` nested within some other namespace here.

================
Comment at: clang-tidy/misc/StdSwapCheck.cpp:78-82
@@ +77,7 @@
+  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");
+
----------------
These lines seem underindented.

================
Comment at: clang-tidy/misc/StdSwapCheck.cpp:84-91
@@ +83,10 @@
+
+  	  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), " }");
+    }
+  }
----------------
This will do bad things if the `std::swap` is not at the start of the enclosing statement (or if there is no enclosing statement).

================
Comment at: docs/clang-tidy/checks/list.rst:35
@@ -34,2 +34,3 @@
    llvm-twine-local
+   misc-StdSwap
    misc-argument-comment
----------------
Why is it called StdSwap here and std-swap below?

================
Comment at: docs/clang-tidy/checks/misc-StdSwap.rst:6
@@ +5,3 @@
+
+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.
+
----------------
practices -> practice
ADL lookup -> ADL / argument dependent lookup / argument dependent name lookup
swap -> `swap`
std -> `std`
"using" statement -> `using` declaration

================
Comment at: docs/clang-tidy/checks/misc-StdSwap.rst:14
@@ +13,3 @@
+
+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.
+
----------------
brackets -> braces [the term "brackets" is ambiguous and implies different things in different dialects]


http://reviews.llvm.org/D15121