aaronpuchert added a comment. I'll add your test case, but I'll probably reuse the existing data structures.
In D68845#1705430 <https://reviews.llvm.org/D68845#1705430>, @Quuxplusone wrote: > Oh, and can you please make sure there are test cases for all the various > cases covered in P1155 > <http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1155r2.html>? > Specifically, I would expect all three of the following test cases to compile > successfully. It looks like they compile successfully in trunk right now > (Godbolt <https://coro.godbolt.org/z/YQ0saN>), so we're just testing that > they don't get broken in the future. > > struct Widget { Widget(); Widget(const Widget&) = delete; Widget(Widget&&); > }; > struct To { operator Widget() &&; }; > task<Widget> nine() { To t; co_return t; } > > struct Fowl { Fowl(Widget); }; > task<Fowl> eleven() { Widget w; co_return w; } > > struct Base { Base(); Base(const Base&) = delete; Base(Base&&); }; > struct Derived : Base {}; > task<Base> thirteen() { Derived result; co_return result; } > These seem to work automatically, because in the end we're just building a function call, which does the right implicit conversions if needed. In D68845#1706193 <https://reviews.llvm.org/D68845#1706193>, @Quuxplusone wrote: > One more test to add: > > struct Widget { > task<Widget> foo() && { > co_return *this; // IIUC this should call return_value(Widget&), > not return_value(Widget&&) > } > }; > We're currently not considering `*this` as implicitly movable, because it's not a DeclRefExpr. ================ 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: > > 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...) > > What do you think about `ImplicitMoveCandidate`? > > I think that would be more correct in this case, but it wouldn't be parallel > with the one in `BuildReturnStmt`, and I'm not sure whether it would be > correct to change it over there too. > > > Isn't it already factored out? > > I see some parallelism in the two other places that call > `getCopyElisionCandidate` followed by `PerformMoveOrCopyInitialization`. > Maybe this is as factored-out as it can get, but it still looks remarkably > parallel. (And I wish `NRVOVariable` was named `NRVOCandidate` in the latter > case.) > > In `BuildReturnStmt`: > > if (RetValExp) > NRVOCandidate = getCopyElisionCandidate(FnRetType, RetValExp, > CES_Strict); > if (!HasDependentReturnType && !RetValExp->isTypeDependent()) { > // we have a non-void function with an expression, continue checking > InitializedEntity Entity = > InitializedEntity::InitializeResult(ReturnLoc, > RetType, > NRVOCandidate != > nullptr); > ExprResult Res = PerformMoveOrCopyInitialization(Entity, NRVOCandidate, > RetType, RetValExp); > > In `BuildCXXThrow`: > > const VarDecl *NRVOVariable = nullptr; > if (IsThrownVarInScope) > NRVOVariable = getCopyElisionCandidate(QualType(), Ex, CES_Strict); > > InitializedEntity Entity = InitializedEntity::InitializeException( > OpLoc, ExceptionObjectTy, > /*NRVO=*/NRVOVariable != nullptr); > ExprResult Res = PerformMoveOrCopyInitialization( > Entity, NRVOVariable, QualType(), Ex, IsThrownVarInScope); > > Naming-wise, I also offer that David Stone's > [P1825](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1825r0.html) > introduces the name "implicitly movable entity" for these things, and > //maybe// we should call this variable `ImplicitlyMovableEntity`; however, I > am not yet sure. > I see some parallelism in the two other places that call > `getCopyElisionCandidate` followed by `PerformMoveOrCopyInitialization`. Note that I'm removing the latter call with this change, and also the call to `InitializedEntity::InitializeResult`: we don't want to initialize anything. Return statements have to actually initialize a return value, but co_return statements only call `<promise>.return_value`. So I think finding the candidate is about as much as we can factor out. (Although it would be nice if it could also catch the case of lvalue references.) ================ Comment at: clang/test/SemaCXX/coroutine-rvo.cpp:71 +task<MoveOnly> param2val(MoveOnly value) { + co_return value; } ---------------- aaronpuchert wrote: > 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' > ``` You are right, a comment says that these are "statements that move coroutine function parameters to the coroutine frame, and store them on the function scope info." I agree with your reading of the draft, it clearly talks about "lvalues". I would guess this is an oversight in the draft though, the moving seems pretty intentional: the statement index is `CoroutineBodyStmt::SubStmt::FirstParamMove`, and `FunctionScopeInfo` has a member `CoroutineParameterMoves`. ================ 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: > aaronpuchert wrote: > > 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). > Okay, that is a reasonable explanation. Me personally, I think overload > resolution is so complicated that you have not necessarily made the test > "stronger," just "different" — the fact that it passes for `Default` does not > at all reassure me that it would necessarily pass for `NCNM` as well. But as > YMMV, I won't press further. Since this is just reference binding it's actually completely irrelevant which constructors are available... so I might as well use `NoCopyNoMove`. Reference binding cannot ever call any constructor, unless I'm mistaken. 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
