[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 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 &Ctx,
+                                            bool IsDecl) {
+  SourceManager &SM = 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