aaronpuchert planned changes to this revision. aaronpuchert added a comment.
In D51741#1702038 <https://reviews.llvm.org/D51741#1702038>, @Quuxplusone wrote: > Both of these should first do overload resolution for one parameter of type > `MoveOnly&&`, and then, only if that overload resolution fails, should they > fall back to overload resolution for one parameter of type `MoveOnly&`. Still need to address that. With this change we're only doing the first step, we still need the fallback. ================ Comment at: clang/lib/Sema/SemaCoroutine.cpp:869 if (E) { - auto NRVOCandidate = this->getCopyElisionCandidate(E->getType(), E, CES_AsIfByStdMove); - if (NRVOCandidate) { - InitializedEntity Entity = - InitializedEntity::InitializeResult(Loc, E->getType(), NRVOCandidate); - ExprResult MoveResult = this->PerformMoveOrCopyInitialization( - Entity, NRVOCandidate, E->getType(), E); - if (MoveResult.get()) - E = MoveResult.get(); - } + VarDecl *NRVOCandidate = + getCopyElisionCandidate(E->getType(), E, CES_Default); ---------------- Quuxplusone wrote: > aaronpuchert wrote: > > Should be renamed to `RVOCandidate`, I don't think NRVO can happen here. > (Btw, I have no comment on the actual code change in this patch. I'm here in > my capacity as > [RVO-explainer](https://www.youtube.com/watch?v=hA1WNtNyNbo)-and-[P1155](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1155r2.html)-author, > not code-understander. ;)) > > What's happening here is never technically "RVO" at all, because there is no > "RV". However, the "N" is accurate. (See [my acronym > glossary](https://quuxplusone.github.io/blog/2019/08/02/the-tough-guide-to-cpp-acronyms/#rvo-nrvo-urvo) > for details.) > The important thing here is that when you say `co_return x;` the `x` is > //named//, and it //would be// a candidate for NRVO if we were in a situation > where NRVO was possible at all. > > The actual optimization that is happening here is "implicit move." > > I would strongly prefer to keep the name `NRVOCandidate` here, because that > is the name that is used for the exact same purpose — computing "implicit > move" candidates — in `BuildReturnStmt`. Ideally this code would be factored > out so that it appeared in only one place; but until then, gratuitous > differences between the two sites should be minimized IMO. Hmm, you're completely right. What do you think about `ImplicitMoveCandidate`? Otherwise I'll stick with the current name, as you suggested. > Ideally this code would be factored out so that it appeared in only one > place; but until then, gratuitous differences between the two sites should be > minimized IMO. Isn't it already factored out? I let `getCopyElisionCandidate` do all the heavy lifting. (Except filtering out lvalue references...) ================ Comment at: clang/test/SemaCXX/coroutine-rvo.cpp:71 +task<MoveOnly> param2val(MoveOnly value) { + co_return value; } ---------------- Quuxplusone wrote: > This should work equally well with `NoCopyNoMove`, right? It should just call > `task<NCNM>::return_value(NCNM&&)`. I don't think you need `MoveOnly` in this > test file anymore. I thought so, too, but there is some code that probably constructs the coroutine and that needs a move constructor. If you look at the AST: ``` FunctionDecl 0xee2e08 <line:70:1, line:72:1> line:70:16 param2val 'task<MoveOnly> (MoveOnly)' |-ParmVarDecl 0xee2cf0 <col:26, col:35> col:35 used value 'MoveOnly' `-CoroutineBodyStmt 0xee7df8 <col:42, line:72:1> |-CompoundStmt 0xee71b8 <line:70:42, line:72:1> | `-CoreturnStmt 0xee7190 <line:71:3, col:13> | |-ImplicitCastExpr 0xee7100 <col:13> 'MoveOnly' xvalue <NoOp> | | `-DeclRefExpr 0xee3088 <col:13> 'MoveOnly' lvalue ParmVar 0xee2cf0 'value' 'MoveOnly' | `-CXXMemberCallExpr 0xee7168 <col:3> 'void' | |-MemberExpr 0xee7138 <col:3> '<bound member function type>' .return_value 0xee5408 | | `-DeclRefExpr 0xee7118 <col:3> 'std::experimental::traits_sfinae_base<task<MoveOnly>, void>::promise_type':'task<MoveOnly>::promise_type' lvalue Var 0xee54e8 '__promise' 'std::experimental::traits_sfinae_base<task<MoveOnly>, void>::promise_type':'task<MoveOnly>::promise_type' | `-ImplicitCastExpr 0xee7100 <col:13> 'MoveOnly' xvalue <NoOp> | `-DeclRefExpr 0xee3088 <col:13> 'MoveOnly' lvalue ParmVar 0xee2cf0 'value' 'MoveOnly' ``` So no move constructor here. But then comes a bunch of other stuff, and finally, ``` `-CoroutineBodyStmt 0xee7df8 <col:42, line:72:1> [...] `-DeclStmt 0xee31f0 <line:71:3> `-VarDecl 0xee3118 <col:3> col:3 implicit used value 'MoveOnly' listinit `-CXXConstructExpr 0xee31c0 <col:3> 'MoveOnly' 'void (MoveOnly &&) noexcept' `-CXXStaticCastExpr 0xee30d8 <col:3> 'MoveOnly' xvalue static_cast<struct MoveOnly &&> <NoOp> `-DeclRefExpr 0xee30a8 <col:3> 'MoveOnly' lvalue ParmVar 0xee2cf0 'value' 'MoveOnly' ``` ================ Comment at: clang/test/SemaCXX/coroutine-rvo.cpp:74 -// expected-no-diagnostics +task<Default> lvalue2val(Default& value) { + co_return value; // expected-error{{rvalue reference to type 'Default' cannot bind to lvalue of type 'Default'}} ---------------- Quuxplusone wrote: > Ditto here, could you use `NoCopyNoMove` instead of `Default`? I used `Default` to show that there is an error even if both copy and move constructor are available, because `return_value` takes a `Default&&` then, but we pass a `Default&` (which is not casted to xvalue). ================ Comment at: clang/test/SemaCXX/coroutine-rvo.cpp:86 + +task<Default&> rvalue2ref(Default&& value) { + co_return value; // expected-error{{non-const lvalue reference to type 'Default' cannot bind to a temporary of type 'Default'}} ---------------- Quuxplusone wrote: > And ditto here: `NoCopyNoMove` instead of `Default`? Also here: because there is an error, testing with `Default` is "stronger". Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68845/new/ https://reviews.llvm.org/D68845 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits