[PATCH] D68845: Don't emit unwanted constructor calls in co_return statements

Fri Oct 11 04:09:09 PDT 2019

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