Quuxplusone marked an inline comment as done. Quuxplusone added inline comments.
================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:23 +def warn_return_std_move : Warning< + "adding 'std::move' here would select a better conversion sequence">, + InGroup<ReturnStdMove>, DefaultIgnore; ---------------- rsmith wrote: > Can we say something like "local variable '%0' will be copied despite being > %select{returned|thrown}1 by name; call 'std::move' explicitly to avoid the > copy"? (Would that be accurate, given the implementation of the warning?) > > Ideally, we'd move the "call 'std::move' explicitly" hint to a separate note > diagnostic and include a FixItHint on that diagnostic to insert the call to > `std::move`. SGTM, except that I don't know how to find out whether we're in the context of a `return` or a `throw` from this deep in the guts of Sema. Could someone give me a pointer on that? I also had trouble figuring out how to generate a fixit from `x` to `std::move(x)` — it keeps coming out as `std::move( )` — but I expect I'll be able to solve that one even without help by banging my head against it a bit. ================ Comment at: include/clang/Sema/Sema.h:3827 bool isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD, - bool AllowParamOrMoveConstructible); + CopyElisionSemanticsKind CESK); ---------------- Q: Is it appropriate for me to be changing the signature of this public method at all? I don't know libclang's rules regarding this kind of change. ================ Comment at: lib/Sema/SemaExprCXX.cpp:731 if (IsThrownVarInScope) - NRVOVariable = getCopyElisionCandidate(QualType(), Ex, false); + NRVOVariable = getCopyElisionCandidate(QualType(), Ex, CES_Strict); ---------------- Q: I'm not sure what concept is being represented by "CES_Strict" here. If there is a more standardese-appropriate name, I'd like to use it. And if there are multiple concepts here that just happen to be identical *coincidentally*, I'd probably prefer to use multiple enumerators that all just happen to have value `0`. But because there are so many call-sites and because this is not directly related to my patch, I have not investigated much. Repository: rC Clang https://reviews.llvm.org/D43322 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits