Index: include/clang/AST/Stmt.h =================================================================== --- include/clang/AST/Stmt.h +++ include/clang/AST/Stmt.h @@ -1344,15 +1344,16 @@ Stmt *RetExpr; SourceLocation RetLoc; const VarDecl *NRVOCandidate; + bool PessimizingMoveCandidate; public: ReturnStmt(SourceLocation RL) : Stmt(ReturnStmtClass), RetExpr(nullptr), RetLoc(RL), - NRVOCandidate(nullptr) {} + NRVOCandidate(nullptr), PessimizingMoveCandidate(false) {} ReturnStmt(SourceLocation RL, Expr *E, const VarDecl *NRVOCandidate) : Stmt(ReturnStmtClass), RetExpr((Stmt*) E), RetLoc(RL), - NRVOCandidate(NRVOCandidate) {} + NRVOCandidate(NRVOCandidate), PessimizingMoveCandidate(false) {} /// \brief Build an empty return expression. explicit ReturnStmt(EmptyShell Empty) : Stmt(ReturnStmtClass, Empty) { } @@ -1372,6 +1373,9 @@ const VarDecl *getNRVOCandidate() const { return NRVOCandidate; } void setNRVOCandidate(const VarDecl *Var) { NRVOCandidate = Var; } + bool isPessimizingMoveCandidate() { return PessimizingMoveCandidate; } + void setPessimizingMoveCandidate() { PessimizingMoveCandidate = true; } + SourceLocation getLocStart() const LLVM_READONLY { return RetLoc; } SourceLocation getLocEnd() const LLVM_READONLY { return RetExpr ? RetExpr->getLocEnd() : RetLoc; Index: include/clang/Basic/DiagnosticGroups.td =================================================================== --- include/clang/Basic/DiagnosticGroups.td +++ include/clang/Basic/DiagnosticGroups.td @@ -279,6 +279,7 @@ def ExplicitInitializeCall : DiagGroup<"explicit-initialize-call">; def Packed : DiagGroup<"packed">; def Padded : DiagGroup<"padded">; +def PessimizingMove : DiagGroup<"pessimizing-move">; def PointerArith : DiagGroup<"pointer-arith">; def PoundWarning : DiagGroup<"#warnings">; def PoundPragmaMessage : DiagGroup<"#pragma-messages">, Index: include/clang/Basic/DiagnosticSemaKinds.td =================================================================== --- include/clang/Basic/DiagnosticSemaKinds.td +++ include/clang/Basic/DiagnosticSemaKinds.td @@ -4741,6 +4741,13 @@ "explicitly moving variable of type %0 to itself">, InGroup, DefaultIgnore; +def warn_pessimizing_move : Warning< + "return value optimization for function %0 prevented by call to " + "std::move on return statement">, + InGroup, DefaultIgnore; +def note_remove_move : Note< + "remove call to std::move here to allow return value optimization">; + def warn_string_plus_int : Warning< "adding %0 to a string does not append to the string">, InGroup; Index: include/clang/Sema/ScopeInfo.h =================================================================== --- include/clang/Sema/ScopeInfo.h +++ include/clang/Sema/ScopeInfo.h @@ -124,6 +124,9 @@ /// false if there is an invocation of an initializer on 'self'. bool ObjCWarnForNoInitDelegation; + /// True when function may contain pessimizing moves in return statements. + bool HasPossiblePessimizingMove; + /// First C++ 'try' statement in the current function. SourceLocation FirstCXXTryLoc; @@ -150,7 +153,7 @@ /// current function scope. These diagnostics are vetted for reachability /// prior to being emitted. SmallVector PossiblyUnreachableDiags; - + /// \brief A list of parameters which have the nonnull attribute and are /// modified in the function. llvm::SmallPtrSet ModifiedNonNullParams; @@ -342,7 +345,7 @@ (HasIndirectGoto || (HasBranchProtectedScope && HasBranchIntoScope)); } - + FunctionScopeInfo(DiagnosticsEngine &Diag) : Kind(SK_Function), HasBranchProtectedScope(false), @@ -354,6 +357,7 @@ ObjCWarnForNoDesignatedInitChain(false), ObjCIsSecondaryInit(false), ObjCWarnForNoInitDelegation(false), + HasPossiblePessimizingMove(true), ErrorTrap(Diag) { } virtual ~FunctionScopeInfo(); Index: include/clang/Sema/Sema.h =================================================================== --- include/clang/Sema/Sema.h +++ include/clang/Sema/Sema.h @@ -1706,7 +1706,8 @@ /// \c constexpr in C++11 or has an 'auto' return type in C++14). bool canSkipFunctionBody(Decl *D); - void computeNRVO(Stmt *Body, sema::FunctionScopeInfo *Scope); + void computeNRVO(Stmt *Body, sema::FunctionScopeInfo *Scope, + FunctionDecl *FD = nullptr); Decl *ActOnFinishFunctionBody(Decl *Decl, Stmt *Body); Decl *ActOnFinishFunctionBody(Decl *Decl, Stmt *Body, bool IsInstantiation); Decl *ActOnSkippedFunctionBody(Decl *Decl); Index: lib/Sema/ScopeInfo.cpp =================================================================== --- lib/Sema/ScopeInfo.cpp +++ lib/Sema/ScopeInfo.cpp @@ -33,6 +33,7 @@ ObjCWarnForNoDesignatedInitChain = false; ObjCIsSecondaryInit = false; ObjCWarnForNoInitDelegation = false; + HasPossiblePessimizingMove = true; FirstCXXTryLoc = SourceLocation(); FirstSEHTryLoc = SourceLocation(); Index: lib/Sema/SemaDecl.cpp =================================================================== --- lib/Sema/SemaDecl.cpp +++ lib/Sema/SemaDecl.cpp @@ -10367,10 +10367,10 @@ } /// \brief Given the set of return statements within a function body, -/// compute the variables that are subject to the named return value +/// compute the variables that are subject to the named return value /// optimization. /// -/// Each of the variables that is subject to the named return value +/// Each of the variables that is subject to the named return value /// optimization will be marked as NRVO variables in the AST, and any /// return statement that has a marked NRVO variable as its NRVO candidate can /// use the named return value optimization. @@ -10378,13 +10378,47 @@ /// This function applies a very simplistic algorithm for NRVO: if every return /// statement in the scope of a variable has the same NRVO candidate, that /// candidate is an NRVO variable. -void Sema::computeNRVO(Stmt *Body, FunctionScopeInfo *Scope) { +void Sema::computeNRVO(Stmt *Body, FunctionScopeInfo *Scope, FunctionDecl *FD) { ReturnStmt **Returns = Scope->Returns.data(); + bool FoundPessimizingMove = false; + const VarDecl *VD = nullptr; + bool HasVD = false; for (unsigned I = 0, E = Scope->Returns.size(); I != E; ++I) { if (const VarDecl *NRVOCandidate = Returns[I]->getNRVOCandidate()) { - if (!NRVOCandidate->isNRVOVariable()) + if (!HasVD) { + VD = NRVOCandidate; + HasVD = true; + } + if (VD != NRVOCandidate) + VD = nullptr; + if (!NRVOCandidate->isNRVOVariable()) { Returns[I]->setNRVOCandidate(nullptr); + if (Returns[I]->isPessimizingMoveCandidate()) { + FoundPessimizingMove = true; + } + } + } + } + + // Only diagnose in functions + if (!FD) return; + // No pessimizing moves found in return statements + if (!FoundPessimizingMove) return; + // No single VarDecl across all return statements + if (!VD) return; + // Check the scope status of pessimizing move + if (!Scope->HasPossiblePessimizingMove) return; + // Don't warn in template instantiations + if (!ActiveTemplateInstantiations.empty()) return; + + Diag(FD->getLocation(), diag::warn_pessimizing_move) << FD; + + for (unsigned I = 0, E = Scope->Returns.size(); I != E; ++I) { + if (Returns[I]->isPessimizingMoveCandidate()) { + Diag(Returns[I]->getLocStart(), diag::note_remove_move) + << FixItHint::CreateReplacement( + Returns[I]->getRetValue()->getSourceRange(), VD->getName()); } } } @@ -10498,15 +10532,15 @@ MarkVTableUsed(FD->getLocation(), Constructor->getParent()); else if (CXXDestructorDecl *Destructor = dyn_cast(FD)) MarkVTableUsed(FD->getLocation(), Destructor->getParent()); - + // Try to apply the named return value optimization. We have to check // if we can do this here because lambdas keep return statements around // to deduce an implicit return type. if (getLangOpts().CPlusPlus && FD->getReturnType()->isRecordType() && !FD->isDependentContext()) - computeNRVO(Body, getCurFunction()); + computeNRVO(Body, getCurFunction(), FD); } - + assert((FD == getCurFunctionDecl() || getCurLambda()->CallOperator == FD) && "Function parsing confused"); } else if (ObjCMethodDecl *MD = dyn_cast_or_null(dcl)) { Index: lib/Sema/SemaStmt.cpp =================================================================== --- lib/Sema/SemaStmt.cpp +++ lib/Sema/SemaStmt.cpp @@ -2905,8 +2905,10 @@ return R; } - if (VarDecl *VD = - const_cast(cast(R.get())->getNRVOCandidate())) { + ReturnStmt *RetStmt = cast(R.get()); + VarDecl *VD = const_cast(RetStmt->getNRVOCandidate()); + // Only set NRVO on true NRVO candidates. Ignore pessimizing move candidates. + if (VD && !RetStmt->isPessimizingMoveCandidate()) { CurScope->addNRVOCandidate(VD); } else { CurScope->setNoNRVO(); @@ -2915,6 +2917,33 @@ return R; } +// If the RetExpr is a function call in the form of std::move(x), return +// the VarDecl for x if "return x;" could possibly be an NRVO. +static VarDecl *GetPessimizingMoveDecl(Sema &SemaRef, QualType RetType, + Expr *RetExpr) { + if (!RetExpr || RetType->isReferenceType()) return nullptr; + + CallExpr *CE = dyn_cast(RetExpr); + if (!CE || CE->getNumArgs() != 1) return nullptr; + + FunctionDecl *FD = CE->getDirectCallee(); + if (!FD || !FD->isInStdNamespace() || !FD->getIdentifier() || + !FD->getIdentifier()->isStr("move")) + return nullptr; + + DeclRefExpr *DRE = dyn_cast(CE->getArg(0)); + if (!DRE || !DRE->getDecl()) return nullptr; + + VarDecl *VD = dyn_cast(DRE->getDecl()); + if (!VD) return nullptr; + + if (!SemaRef.isCopyElisionCandidate(RetType, VD, false)) return nullptr; + + if (!SemaRef.Context.hasSameType(RetType, VD->getType())) return nullptr; + + return VD; +} + StmtResult Sema::BuildReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp) { // Check for unexpanded parameter packs. if (RetValExp && DiagnoseUnexpandedParameterPack(RetValExp)) @@ -2928,6 +2957,9 @@ const AttrVec *Attrs = nullptr; bool isObjCMethod = false; + // Hold the VarDecl being moved in the return statement. + VarDecl* MovedDecl = nullptr; + if (const FunctionDecl *FD = getCurFunctionDecl()) { FnRetType = FD->getReturnType(); if (FD->hasAttrs()) @@ -2935,6 +2967,7 @@ if (FD->isNoReturn()) Diag(ReturnLoc, diag::warn_noreturn_function_has_return_expr) << FD->getDeclName(); + MovedDecl = GetPessimizingMoveDecl(*this, FnRetType, RetValExp); } else if (ObjCMethodDecl *MD = getCurMethodDecl()) { FnRetType = MD->getReturnType(); isObjCMethod = true; @@ -3120,10 +3153,20 @@ Result = new (Context) ReturnStmt(ReturnLoc, RetValExp, NRVOCandidate); } - // If we need to check for the named return value optimization, save the - // return statement in our scope for later processing. - if (Result->getNRVOCandidate()) + if (Result->getNRVOCandidate()) { + // If we need to check for the named return value optimization, save the + // return statement in our scope for later processing. FunctionScopes.back()->Returns.push_back(Result); + } else if (MovedDecl) { + // If the return expression is "return std::move(x);" and "return x;" could + // be a NRVO, mark it as a pessimizing move for later diagnostics. + Result->setNRVOCandidate(MovedDecl); + Result->setPessimizingMoveCandidate(); + FunctionScopes.back()->Returns.push_back(Result); + } else { + // Return statement is neither an NRVO candidate or a pessimizing move. + FunctionScopes.back()->HasPossiblePessimizingMove = false; + } return Result; } Index: test/SemaCXX/warn-pessimizing-move.cpp =================================================================== --- test/SemaCXX/warn-pessimizing-move.cpp +++ test/SemaCXX/warn-pessimizing-move.cpp @@ -0,0 +1,98 @@ +// RUN: %clang_cc1 -fsyntax-only -Wpessimizing-move -std=c++11 -verify %s + +// definitions for std::move +namespace std { +inline namespace foo { +template struct remove_reference { typedef T type; }; +template struct remove_reference { typedef T type; }; +template struct remove_reference { typedef T type; }; + +template typename remove_reference::type &&move(T &&t); +} +} + +struct S {}; + +// One move return statement +S test1() { + // expected-warning@-1 {{return value optimization for function 'test1' prevented by call to std::move on return statement}} + S s; + return std::move(s); + // expected-note@-1 {{remove call to std::move here to allow return value optimization}} +} + +// Two move return statements +S test2(int x) { + // expected-warning@-1 {{return value optimization for function 'test2' prevented by call to std::move on return statement}} + S s; + if (x > 5) { + return std::move(s); + // expected-note@-1 {{remove call to std::move here to allow return value optimization}} + } else { + return std::move(s); + // expected-note@-1 {{remove call to std::move here to allow return value optimization}} + } +} + +// One move return, one regular return +S test3(int x) { + // expected-warning@-1 {{return value optimization for function 'test3' prevented by call to std::move on return statement}} + S s; + if (x > 5) { + return s; + } else { + return std::move(s); + // expected-note@-1 {{remove call to std::move here to allow return value optimization}} + } +} + +// One move return, one return not valid for return value optimization +S test4(int x) { + S s; + if (x > 5) { + return S(); + } else { + return std::move(s); + } +} + +// Not valid for NRVO since not all returns use a local variable +S test5(int x) { + S s; + if (x > 5) { + return S(); + } else { + return s; + } +} + +// Not pessimizing move since moved variables are different +S test6(int x) { + S s1, s2; + if (x > 5) { + return std::move(s1); + } else { + return std::move(s2); + } +} + +// Don't warn in templates +template +S test7() { + S s; + return std::move(s); +} + +void test8() { + S s = test7(); +} + +template +T test9() { + T t; + return std::move(t); +} + +void test10() { + S s = test9(); +}