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