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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
