[PATCH] Clang-Modernize.Pass-By-Value: Fix removing of namespace in parameter type

[Guillaume Papin]

  I changed the way I handled things since my first submission. Now the
  pass-by-value transform will try to transform only const-ref parameters and
  non-const value parameters.

  The test have been updated to reflect the changes.

Hi revane, arielbernal, tareqsiraj,

http://llvm-reviews.chandlerc.com/D1600

CHANGE SINCE LAST DIFF
  http://llvm-reviews.chandlerc.com/D1600?vs=4040&id=4074#toc

Files:
  clang-modernize/PassByValue/PassByValueActions.cpp
  clang-modernize/PassByValue/PassByValueMatchers.cpp
  test/clang-modernize/PassByValue/basic.cpp

Index: clang-modernize/PassByValue/PassByValueActions.cpp
===================================================================
--- clang-modernize/PassByValue/PassByValueActions.cpp
+++ clang-modernize/PassByValue/PassByValueActions.cpp
@@ -113,16 +113,20 @@
   collectParamDecls(Ctor, ParamDecl, AllParamDecls);
 
   // Generate all replacements for the params.
-  llvm::SmallVector<Replacement, 2> ParamReplaces(AllParamDecls.size());
+  llvm::SmallVector<Replacement, 2> ParamReplaces;
   for (unsigned I = 0, E = AllParamDecls.size(); I != E; ++I) {
     TypeLoc ParamTL = AllParamDecls[I]->getTypeSourceInfo()->getTypeLoc();
+    ReferenceTypeLoc RefTL = ParamTL.getAs<ReferenceTypeLoc>();
     SourceRange Range(AllParamDecls[I]->getLocStart(), ParamTL.getLocEnd());
     CharSourceRange CharRange = Lexer::makeFileCharRange(
         CharSourceRange::getTokenRange(Range), SM, LangOptions());
-    TypeLoc ValueTypeLoc = ParamTL;
+
+    // do not generate a replacement when the parameter is already a value
+    if (RefTL.isNull())
+      continue;
+
     // transform non-value parameters (e.g: const-ref) to values
-    if (!ParamTL.getNextTypeLoc().isNull())
-      ValueTypeLoc = ParamTL.getNextTypeLoc();
+    TypeLoc ValueTypeLoc = RefTL.getPointeeLoc();
     llvm::SmallString<32> ValueStr = Lexer::getSourceText(
         CharSourceRange::getTokenRange(ValueTypeLoc.getSourceRange()), SM,
         LangOptions());
@@ -136,7 +140,7 @@
     // 'const Foo &param' -> 'Foo param'
     //  ~~~~~~~~~~~           ~~~^
     ValueStr += ' ';
-    ParamReplaces[I] = Replacement(SM, CharRange, ValueStr);
+    ParamReplaces.push_back(Replacement(SM, CharRange, ValueStr));
   }
 
   // Reject the changes if the the risk level is not acceptable.
Index: clang-modernize/PassByValue/PassByValueMatchers.cpp
===================================================================
--- clang-modernize/PassByValue/PassByValueMatchers.cpp
+++ clang-modernize/PassByValue/PassByValueMatchers.cpp
@@ -63,15 +63,29 @@
 using namespace clang;
 using namespace clang::ast_matchers;
 
+static TypeMatcher constRefType() {
+  return lValueReferenceType(pointee(isConstQualified()));
+}
+
+static TypeMatcher nonConstValueType() {
+  return qualType(unless(anyOf(referenceType(), isConstQualified())));
+}
+
 DeclarationMatcher makePassByValueCtorParamMatcher() {
   return constructorDecl(
       forEachConstructorInitializer(ctorInitializer(
           // Clang builds a CXXConstructExpr only when it knowns which
           // constructor will be called. In dependent contexts a ParenListExpr
           // is generated instead of a CXXConstructExpr, filtering out templates
           // automatically for us.
           withInitializer(constructExpr(
-              has(declRefExpr(to(parmVarDecl().bind(PassByValueParamId)))),
+              has(declRefExpr(to(
+                  parmVarDecl(hasType(qualType(
+                                  // match only const-ref or a non-const value
+                                  // parameters, rvalues and const-values
+                                  // shouldn't be modified.
+                                  anyOf(constRefType(), nonConstValueType()))))
+                      .bind(PassByValueParamId)))),
               hasDeclaration(constructorDecl(
                   isNonDeletedCopyConstructor(),
                   hasDeclContext(recordDecl(isMoveConstructible())))))))
Index: test/clang-modernize/PassByValue/basic.cpp
===================================================================
--- test/clang-modernize/PassByValue/basic.cpp
+++ test/clang-modernize/PassByValue/basic.cpp
@@ -72,11 +72,17 @@
 // Test unnamed parameter in declaration
 struct G {
   G(const Movable &);
-  // CHECK: G(Movable );
+  // CHECK: G(Movable);
+
+  // Test that a space is not added unnecessarily before a comma
+  G(const Movable &, int);
+  // CHECK: G(Movable, int);
   Movable M;
 };
 G::G(const Movable &M) : M(M) {}
 // CHECK: G::G(Movable M) : M(std::move(M)) {}
+G::G(const Movable &M, int) : M(M) {}
+// CHECK: G::G(Movable M, int) : M(std::move(M)) {}
 
 // Test parameter with and without qualifier
 namespace ns_H {
@@ -150,7 +156,7 @@
 // Test with a const-value parameter
 struct P {
   P(const Movable M) : M(M) {}
-  // CHECK: P(Movable M) : M(std::move(M)) {}
+  // CHECK: P(const Movable M) : M(M) {}
   Movable M;
 };
 
@@ -166,3 +172,20 @@
   Movable C;
   double D;
 };
+
+// Test that value-parameters with a nested name specifier are left as-is
+namespace ns_R {
+typedef ::Movable RMovable;
+}
+struct R {
+  R(ns_R::RMovable M) : M(M) {}
+  // CHECK: R(ns_R::RMovable M) : M(std::move(M)) {}
+  ns_R::RMovable M;
+};
+
+// Test with rvalue parameter
+struct S {
+  S(Movable &&M) : M(M) {}
+  // CHECK: S(Movable &&M) : M(M) {}
+  Movable M;
+};
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D1600.2.patch
Type: text/x-patch
Size: 4814 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130905/6923618c/attachment.bin>