diff options
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 ¶m' -> '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; +}; |
