[PATCH] D51741: [coro]Pass rvalue reference for named local variable to return_value

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

Quuxplusone added inline comments.


================
Comment at: cfe/trunk/lib/Sema/SemaCoroutine.cpp:857-859
   // FIXME: If the operand is a reference to a variable that's about to go out
   // of scope, we should treat the operand as an xvalue for this overload
   // resolution.
----------------
aaronpuchert wrote:
> @Quuxplusone Am I right that your paper basically addresses this comment?
I'm not yet motivated enough to look closely enough to give an //informed// opinion... but my kneejerk impression is that this FIXME comment is saying "we should do implicit move [in the [P1155](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1155r2.html) sense] here." In which case, this patch (D51741) itself fixed this FIXME at least partly, and maybe completely. Maybe this patch should have removed or amended the FIXME, rather than just adding code above it.

> But since you're here: what is the right `CopyElisionSemanticsKind`, is it `CES_AsIfByStdMove` like this change does it, or `CES_Strict` like `BuildReturnStmt` does it?

Oh geez, asking me about code I introduced... ;) My impression is that the correct thing here is `CES_Strict`.

Notice that we have a `CES_FormerDefault` (basically "what were the rules in C++11 before the first DR"), and then a `CES_Default` (basically "what are the rules in C++17, before [P1825](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1825r0.html)"). My `-Wreturn-std-move` patch added `CES_AsIfByStdMove` as an **implementation detail** of the diagnostic codepath. I didn't expect anyone to use it in real code.

However, now that [P1825](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1825r0.html) has been accepted into C++2a, `CES_AsIfByStdMove` is the actual (draft-)standard behavior, and should rightly be named something like `CES_FutureDefault`!

So I would suggest that this code should use `CES_Strict` for now, just like we do in `BuildReturnStmt`. Then, someone should "implement [P1825](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1825r0.html)," which means figuring out what the heck to do about `-Wreturn-std-move`... but whatever they figure out, it'll apply equally cleanly to this code as to `BuildReturnStmt`.

I'm confused because both here and `BuildReturnStmt` have calls to `getCopyElisionCandidate` //outside// the call to `PerformMoveOrCopyInitialization`, even though `PerformMoveOrCopyInitialization` will happily accept a null `NRVOCandidate` as a signal to make its own call to `getCopyElisionCandidate`. (And notice that //that// call will use `CES_Default`, which seems right, as opposed to `CES_Strict`, which seems wrong to me, but clearly I've forgotten how this stuff works.)


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51741/new/

https://reviews.llvm.org/D51741