[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 added inline comments.


================
Comment at: lib/Sema/SemaStmt.cpp:2937
 
+static void AttemptMoveInitialization(Sema& S,
+                                      const InitializedEntity &Entity,
----------------
rtrieu wrote:
> I have a few concerns about this function.  The code isn't a straight move from the old location to this one, so it is hard to follow along the changes, especially the change to the complex if conditional.  The function should be commented, especially to behavior difference for setting IsFake.
> 
> It looks like when IsFake is set, then the result is discarded and not used, but it is still possible on success for AsRvalue to be copied to the heap.  This is wasteful when it is never used again.
> 
> Another issue is the Value in the original code is used again towards the end of the function on line #3013.  In the old code, Value can be updated while in the new code, it does.
> 
> It may be better to split this change in two, the first adding this function and the CopyElisionSemanticsKind enum and the second adding the diagnostic itself.
Hi @rtrieu, and thanks!

I have split out the first half of the patch into a new revision D43898, and updated this one with the full patch (both halves together). Is there an easy way for me to make "just the second half" reviewable on its own, before the first half has been merged to master?

> It looks like when IsFake is set, then the result is discarded and not used, but it is still possible on success for AsRvalue to be copied to the heap. This is wasteful when it is never used again.

I believe you are correct. But I'm not sure if it's safe to use `AsRvalue` as input to `Res` (which *is* used outside this function) if it's not moved like this; I don't know much about the internals here. You or anyone have a suggestion for how to fix that issue?

> In the old code, Value can be updated while in the new code, it does.

I can't parse this, sorry.

FYI, in the patch I'm about to upload, I have renamed `!IsFake` to `ConvertingConstructorsOnly`, which should be more pleasing to the eye. :)


Repository:
  rC Clang

https://reviews.llvm.org/D43322