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

Reply via email to