clang-modernize: Fix bugs in Pass-By-Value transform

- Limit the transform to const-ref and non-const value parameters only.
- Do not generate a replacement when the type is already a value.

See CM-139 for the bugs corresponding to this issue.

llvm-svn: 190212

3 files changed, 42 insertions, 7 deletions

diff --git a/clang-tools-extra/clang-modernize/PassByValue/PassByValueActions.cpp b/clang-tools-extra/clang-modernize/PassByValue/PassByValueActions.cpp
index 63189219294..e9f98e744c9 100644
--- a/clang-tools-extra/clang-modernize/PassByValue/PassByValueActions.cpp
+++ b/clang-tools-extra/clang-modernize/PassByValue/PassByValueActions.cpp
@@ -113,16 +113,20 @@ void ConstructorParamReplacer::run(const MatchFinder::MatchResult &Result) {
   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 @@ void ConstructorParamReplacer::run(const MatchFinder::MatchResult &Result) {
     // '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.
diff --git a/clang-tools-extra/clang-modernize/PassByValue/PassByValueMatchers.cpp b/clang-tools-extra/clang-modernize/PassByValue/PassByValueMatchers.cpp
index 32095e167b7..1a0cbcf5ef9 100644
--- a/clang-tools-extra/clang-modernize/PassByValue/PassByValueMatchers.cpp
+++ b/clang-tools-extra/clang-modernize/PassByValue/PassByValueMatchers.cpp
@@ -63,6 +63,14 @@ AST_MATCHER(CXXConstructorDecl, isNonDeletedCopyConstructor) {
 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(
@@ -71,7 +79,13 @@ DeclarationMatcher makePassByValueCtorParamMatcher() {
           // 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())))))))
diff --git a/clang-tools-extra/test/clang-modernize/PassByValue/basic.cpp b/clang-tools-extra/test/clang-modernize/PassByValue/basic.cpp
index 118b3663d55..0b0c1ea8d95 100644
--- a/clang-tools-extra/test/clang-modernize/PassByValue/basic.cpp
+++ b/clang-tools-extra/test/clang-modernize/PassByValue/basic.cpp
@@ -150,7 +150,7 @@ struct O {
 // 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 +166,20 @@ struct Q {
   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;
+};