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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.


================
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);
----------------
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.


================
Comment at: clang/test/SemaCXX/coroutine-rvo.cpp:71
+task<MoveOnly> param2val(MoveOnly value) {
+  co_return value;
 }
----------------
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.


================
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'}}
----------------
Ditto here, could you use `NoCopyNoMove` instead of `Default`?


================
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'}}
----------------
And ditto here: `NoCopyNoMove` instead of `Default`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68845