[PATCH] D43322: Diagnose cases of "return x" that should be "return std::move(x)" for efficiency

[Arthur O'Dwyer via Phabricator via cfe-commits]

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