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

Wed Oct 9 13:32:05 PDT 2019

aaronpuchert added a comment.

In D51741#1702038 <https://reviews.llvm.org/D51741#1702038>, @Quuxplusone wrote:

> This patch is heavily heavily merge-conflicted by P1825 <http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1825r0.html>.

Just to be clear, this change is pretty old: it's already contained in Clang 8. I was just adding comments.

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?

================
Comment at: cfe/trunk/lib/Sema/SemaCoroutine.cpp:846
+  if (E) {
+    auto NRVOCandidate = this->getCopyElisionCandidate(E->getType(), E, CES_AsIfByStdMove);
+    if (NRVOCandidate) {
----------------
modocache wrote:
> aaronpuchert wrote:
> > Why not `CES_Strict` like in `Sema::BuildReturnStmt`? With `CES_Strict` the test still works, and we can also return references.
> So this fixes your test case? If so it sounds good to me. I'll make this change or you can feel free to if you get around to it first.
I can post a change, I'm just not sure if it's correct. The (Clang) tests run fine, will test with libc++ later today or tomorrow.

================
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.
----------------
@Quuxplusone Am I right that your paper basically addresses this comment?

Repository:
  rL LLVM

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

https://reviews.llvm.org/D51741