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

[Aaron Ballman via cfe-commits]

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 &Ctx,
----------------
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 <utility>
+
----------------
It would be good to not have the #include <utility> 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