[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.


================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:1585-1586
   InitializedEntity Entity = InitializedEntity::InitializeVariable(GroDecl);
-  ExprResult Res = S.PerformMoveOrCopyInitialization(Entity, nullptr, GroType,
-                                                     this->ReturnValue);
+  ExprResult Res =
+      S.PerformCopyInitialization(Entity, SourceLocation(), ReturnValue);
   if (Res.isInvalid())
----------------
mizvekov wrote:
> mizvekov wrote:
> > aaronpuchert wrote:
> > > Perhaps this should just be direct initialization? Can't really find anything in the standard though.
> > So I just checked this again. Regarding our conversation on IRC, what I said initially was correct: `ReturnValue` is always a member function expression, built by `makeReturnObject` -> `buildPromiseCall` -> `buildMemberCall`. So implicit move would never trigger here, and as far as I see there is no reason for this code to have used PerformMoveOrCopyInitialization in the first place.
> Err I meant: member function *call* expression
Also, I don't think we could force direct initialization here, if the object returned by Gro is volatile for example.
With that said, copy elision from the sema action on the return statement of get_return_object should take care of removing this copy here, I think, I don't see any reason it would not work here just as well as for expressions coming from the parser.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:854
   if (Ex && !Ex->isTypeDependent()) {
+    NRVOResult NRVORes = IsThrownVarInScope ? getNRVOResult(Ex) : NRVOResult();
+
----------------
mizvekov wrote:
> mizvekov wrote:
> > aaronpuchert wrote:
> > > Any reason why you've moved that away from the comment that wants to explain it?
> > Yes, on C++2b this might modify Ex, and I moved it so this would happen before we do the check with the type of the expression just below here.
> But yeah I forgot to move the comment, good catch.
Actually, had my wires crossed there with the P2266 DR, I only need to move this line there.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3157-3159
+      // it would deduce to a reference.
+      if (AT->isDecltypeAuto() && Res.isParenthesized)
+        return Res = NRVOResult(), void();
----------------
aaronpuchert wrote:
> Can't we just do this when we know what it deduces to? It sounds weird to handle dependent contexts here because we shouldn't attempt any return value initialization then.
I think there are problems regarding the limitations of the current NRVO algorithm, but I had not had much time to study it or think of improvements, so I follow the current approach of trying to use as much available information as possible in order to eliminate candidates early.

With that said, this is not well covered by the test suite and I could probably get away with a lot less without breaking any existing tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696