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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
