[PATCH] D68845: Don't emit unwanted constructor calls in co_return statements
Aaron Puchert via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sat Oct 12 07:51:43 PDT 2019
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
More information about the cfe-commits
mailing list