Re: [PATCH] D15121: A new clang-tidy module to find calls to `std::swap`, and change them to use ADL
Prazek added a subscriber: Prazek.
Comment at: test/clang-tidy/misc-StdSwap.cpp:7-11
@@ +6,7 @@
+
+// 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.
+
template thing
http://reviews.llvm.org/D15121
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15121: A new clang-tidy module to find calls to `std::swap`, and change them to use ADL
aaron.ballman added a comment.
I notice there are a few other comments from reviewers that have not been
addressed -- is there a newer version of the patch that hasn't been uploaded
yet, or are you looking for further information on some of the comments?
Comment at: clang-tidy/misc/StdSwapCheck.cpp:26
@@ +25,3 @@
+ASTContext ,
+bool IsDecl) {
+ SourceManager = Ctx.getSourceManager();
> We should put that somewhere into Tooling/Core. Adding Benjamin who is our
> master of Yaks :D
At the very least, this could live in clangTidyUtils, possibly in LexerUtils.h.
Comment at: clang-tidy/misc/StdSwapCheck.cpp:65
@@ +64,3 @@
+ Finder->addMatcher(
+//callExpr(callee(functionDecl(hasName("swap"))),
+callExpr(callee(namedDecl(hasName("swap"))),
Please remove commented out code.
Comment at: clang-tidy/misc/StdSwapCheck.cpp:77
@@ +76,3 @@
+
+// bool isGlobalNS =
NamespaceDecl::castToDeclContext(nsNode->getNestedNameSpecifier()->getAsNamespace())->getParent()->isTranslationUnit();
+// if (isGlobalNS && (nsStr == "std" || nsStr == "::std")) {
Please remove commented-out code (here and elsewhere).
Comment at: clang-tidy/misc/StdSwapCheck.h:20
@@ +19,3 @@
+///with calls that use ADL instead.
+///
+/// For the user-facing documentation see:
> I like that indentation :-)
> It indicates that this is a continuation of the previous line.
It's not the style of commenting that we use in the rest of the project,
however.
http://reviews.llvm.org/D15121
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15121: A new clang-tidy module to find calls to `std::swap`, and change them to use ADL
mclow.lists added a comment. ping? http://reviews.llvm.org/D15121 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15121: A new clang-tidy module to find calls to `std::swap`, and change them to use ADL
alexfh added a subscriber: alexfh. alexfh added a comment. Found this accidentally, since I don't read all of cfe-commits normally. Would be nice if you could add me to reviewers on clang-tidy-related patches. Thanks! In http://reviews.llvm.org/D15121#300837, @dblaikie wrote: > (tangential: Should we just have a utility function llvm::adl_swap that we > could use everywhere rather than having to write two lines for every > caller? (& have a variant of this check that knows to suggest that in > LLVM?)) FWIW, if this seems to be useful for other codebases, the name of this function could be made configurable (together with the header this function comes from - clang-tidy checks can use `clang::tidy::IncludeInserter` to insert includes in a proper way). http://reviews.llvm.org/D15121 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15121: A new clang-tidy module to find calls to `std::swap`, and change them to use ADL
mclow.lists added a comment. I ran this on LLVM + clang, and it changed calls in 61 files. The changed LLVM codebase compiled successfully, and passed all the tests. Re: @dblaikie's comment, I'd rather call such a beast `llvm::swap`, and it would have to go into a header file that everyone already includes. http://reviews.llvm.org/D15121 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15121: A new clang-tidy module to find calls to `std::swap`, and change them to use ADL
On Thu, Dec 3, 2015 at 8:08 AM, Marshall Clow via cfe-commits < cfe-commits@lists.llvm.org> wrote: > mclow.lists added a comment. > > I ran this on LLVM + clang, and it changed calls in 61 files. The changed > LLVM codebase compiled successfully, and passed all the tests. > > Re: @dblaikie's comment, I'd rather call such a beast `llvm::swap`, Strikes me as a little subtle, but I don't find it fundamentally objectionable. > and it would have to go into a header file that everyone already includes. > Fair - difficult to do fixits that add includes. And while we do have a few headers that nearly everyone includes, they're probably not /everywhere/ and it may not be appropriate to put this utility in them anyway. Perhaps we can just, as a rule, ignore the general fixit hint and fix it 'our' way? (or could we (do we?) have a lower bar for clang-tidy fixit hints that would allow us to just fixit to this function and let the user realize they need to add a missing include?) > > > http://reviews.llvm.org/D15121 > > > > ___ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15121: A new clang-tidy module to find calls to `std::swap`, and change them to use ADL
aaron.ballman added inline comments. Comment at: clang-tidy/misc/StdSwapCheck.cpp:86 @@ +85,3 @@ + SourceLocation semiLoc = findSemiAfterLocation(swapSourceRange.getEnd(), *Result.Context, false); + assert(semiLoc != SourceLocation() && "Can't find the terminating semicolon" ); + Extra space at the end of the assert before the closing paren. Comment at: clang-tidy/misc/StdSwapCheck.h:19 @@ +18,3 @@ +/// A checker that finds ns-qualified calls to std::swap, and replaces them +///with calls that use ADL instead. +/// Strange indentation on the second line of the comment. Comment at: test/clang-tidy/misc-StdSwap.cpp:4 @@ +3,3 @@ +#include + +// FIXME: Add something that triggers the check here. > The reason that is included here is that that is where swap is > declared. The usual approach we take in our tests is to declare the STL functionality inside of the source file being tested. For instance, see misc-move-constructor-init.cpp. Another approach, if you require the declaration to be in a header file (which you don't appear to from the checker) is to create a local test file and #include it with "header" instead of . Relying on whatever STL happens to be found by header search logic makes the tests highly fragile (and bots will happily chirp at you when you do this). http://reviews.llvm.org/D15121 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15121: A new clang-tidy module to find calls to `std::swap`, and change them to use ADL
mclow.lists added inline comments. Comment at: clang-tidy/misc/StdSwapCheck.cpp:24 @@ +23,3 @@ +/// source location will be invalid. +static SourceLocation findSemiAfterLocation(SourceLocation loc, +ASTContext , aaron.ballman wrote: > rsmith wrote: > > Is there somewhere more central where this can live? > If it is useful to multiple checkers, it could live in clangTidyUtils, or > were you thinking of something more general for clang itself? I'd love to have this in a library somewhere; I found a discussion about this from a year ago; Manuel seemed to think that it was a good idea, but nothing apparently came of that. I lifted this code from `llvm/tools/clang/lib/ARCMigrate/Transforms.cpp` Comment at: test/clang-tidy/misc-StdSwap.cpp:3 @@ +2,3 @@ + +#include + aaron.ballman wrote: > It would be good to not have the #include here -- for instance, I > think that this test will fail on Windows with MSVC if the only MSVC STL > headers that can be found are from VS 2015 because there's no > -fms-compatibility-version=19 option being used. The reason that is included here is that that is where `swap` is declared. http://reviews.llvm.org/D15121 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15121: A new clang-tidy module to find calls to `std::swap`, and change them to use ADL
klimek added a reviewer: bkramer.
Comment at: clang-tidy/misc/StdSwapCheck.cpp:25
@@ +24,3 @@
+static SourceLocation findSemiAfterLocation(SourceLocation loc,
+ASTContext ,
+bool IsDecl) {
We should put that somewhere into Tooling/Core. Adding Benjamin who is our
master of Yaks :D
http://reviews.llvm.org/D15121
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15121: A new clang-tidy module to find calls to `std::swap`, and change them to use ADL
aaron.ballman added inline comments.
Comment at: clang-tidy/misc/StdSwapCheck.cpp:24
@@ +23,3 @@
+/// source location will be invalid.
+static SourceLocation findSemiAfterLocation(SourceLocation loc,
+ASTContext ,
rsmith wrote:
> Is there somewhere more central where this can live?
If it is useful to multiple checkers, it could live in clangTidyUtils, or were
you thinking of something more general for clang itself?
Comment at: clang-tidy/misc/StdSwapCheck.cpp:86
@@ +85,3 @@
+ SourceLocation semiLoc =
findSemiAfterLocation(swapSourceRange.getEnd(), *Result.Context, false);
+ assert(semiLoc != SourceLocation());
+
Can we get an && message in here as to what is being asserted?
Comment at: clang-tidy/misc/StdSwapCheck.cpp:89
@@ +88,3 @@
+ nsSourceRange.setEnd(nsSourceRange.getEnd().getLocWithOffset(2));
+ Diag << FixItHint::CreateReplacement(nsSourceRange, "{ using std::swap;
")
+ << FixItHint::CreateInsertion(semiLoc.getLocWithOffset(1), " }");
I wonder if there is a way to guard against code like:
```
SomeStruct S1, S2;
if (something) { std::swap(S1, S2); }
```
from becoming:
```
SomeStruct S1, S2;
if (something) {{ using std::swap; swap(S3, S4); }}
```
Also, should the fixit be suppressed under some circumstances?
```
// Is the replacement always safe for all macro expansions?
#define SWAP(a, b) std::swap(a, b)
// I should probably be punished for considering this code
for (;;std::swap(a, b)) ;
if (std::swap(a, b), (bool)a) ;
```
Comment at: clang-tidy/misc/StdSwapCheck.h:18
@@ +17,3 @@
+
+/// FIXME: Write a short description.
+///
Can this FIXME be fixed? ;-)
Comment at: docs/clang-tidy/checks/misc-StdSwap.rst:1
@@ +1,2 @@
+misc-StdSwap
+
This is incorrect, as is the file name (at least, compared to the comments in
StdSwapCheck.h. Can this (and the file) be renamed to misc-std-swap instead?
Comment at: test/clang-tidy/misc-StdSwap.cpp:3
@@ +2,3 @@
+
+#include
+
It would be good to not have the #include here -- for instance, I
think that this test will fail on Windows with MSVC if the only MSVC STL
headers that can be found are from VS 2015 because there's no
-fms-compatibility-version=19 option being used.
Comment at: test/clang-tidy/misc-StdSwap.cpp:61
@@ +60,3 @@
+bar::swap(i,j);
+}
+
It would also be good to have a test that checks bar::std::swap(a, b) doesn't
get flagged.
http://reviews.llvm.org/D15121
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15121: A new clang-tidy module to find calls to `std::swap`, and change them to use ADL
mclow.lists added inline comments.
Comment at: clang-tidy/misc/StdSwapCheck.cpp:68
@@ +67,3 @@
+
callee(expr(ignoringParenImpCasts(declRefExpr(has(nestedNameSpecifierLoc().bind("namespace"))).bind("swap"),
+this);
+}
I believe that if you do that, you lose the arguments as well.
std::swap(x,y); -> { using ::std::swap; swap; }
Comment at: clang-tidy/misc/StdSwapCheck.h:19
@@ +18,3 @@
+/// A checker that finds ns-qualified calls to std::swap, and replaces them
+///with calls that use ADL instead.
+///
aaron.ballman wrote:
> Strange indentation on the second line of the comment.
I like that indentation :-)
It indicates that this is a continuation of the previous line.
http://reviews.llvm.org/D15121
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15121: A new clang-tidy module to find calls to `std::swap`, and change them to use ADL
mclow.lists updated this revision to Diff 41667.
mclow.lists added a comment.
Richard clued me in to the cool method `isStdNamespace()`, which made the code
simpler.
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 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 ) { 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
Re: [PATCH] D15121: A new clang-tidy module to find calls to `std::swap`, and change them to use ADL
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 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 ) { 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.
+//
+//===--===//
Re: [PATCH] D15121: A new clang-tidy module to find calls to `std::swap`, and change them to use ADL
(tangential: Should we just have a utility function llvm::adl_swap that we could use everywhere rather than having to write two lines for every caller? (& have a variant of this check that knows to suggest that in LLVM?)) On Wed, Dec 2, 2015 at 1:35 PM, Marshall Clow via cfe-commits < cfe-commits@lists.llvm.org> wrote: > mclow.lists marked 5 inline comments as done. > mclow.lists added a comment. > > http://reviews.llvm.org/D15121 > > > > ___ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15121: A new clang-tidy module to find calls to `std::swap`, and change them to use ADL
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
+
+// 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 ) { 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
Re: [PATCH] D15121: A new clang-tidy module to find calls to `std::swap`, and change them to use ADL
LegalizeAdulthood added a subscriber: LegalizeAdulthood. LegalizeAdulthood added a comment. 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? http://reviews.llvm.org/D15121 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15121: A new clang-tidy module to find calls to `std::swap`, and change them to use ADL
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 ,
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("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("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
[PATCH] D15121: A new clang-tidy module to find calls to `std::swap`, and change them to use ADL
mclow.lists created this revision.
mclow.lists added reviewers: klimek, aaron.ballman, chandlerc.
mclow.lists added a subscriber: cfe-commits.
Motivation: LLVM has many overloads of `std::swap` for its own types. This is
technically forbidden by the standard, but is pervasive in the LLVM code base.
The correct fix for this is to put them in the same namespace as the thing that
they are swapping - but do do this, we have to find (and fix) all the places
where we call `std::swap` explicitly, and let ADL do the finding instead.
The basic transform is:
std::swap(x, y) --> { using std::swap; swap(x, y); }
This is the first cut at a tool to do this.
It's not quite right, in that it does this instead:
std::swap(x, y) --> { using std::swap; :swap(x, y); }
and I'm not quite sure why.
Also, the checking for builtin types and pointers needs to go into the matcher
instead of the code.
Thanks to Manuel and Aaron for helping with this
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,63 @@
+// RUN: %check_clang_tidy %s misc-StdSwap %t
+
+#include
+
+// 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 ) { 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]]:31: 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]]:31: 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++ -*-===//
+//
+//
Re: [PATCH] D15121: A new clang-tidy module to find calls to `std::swap`, and change them to use ADL
mclow.lists added a comment. Also, even though the `--fix` done by the tool is (almost) correct, the display of the fixit hint by clang is really confusing, even wrong http://reviews.llvm.org/D15121 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
