[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())
----------------
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.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:854
   if (Ex && !Ex->isTypeDependent()) {
+    NRVOResult NRVORes = IsThrownVarInScope ? getNRVOResult(Ex) : NRVOResult();
+
----------------
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.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3030-3032
+  Res.S = Res.S != Sema::NRVOResult::None && CanMove
+              ? Sema::NRVOResult::MoveEligible
+              : Sema::NRVOResult::None;
----------------
aaronpuchert wrote:
> The way you've designed the enumeration, couldn't you use `std::min` here?
Yes exactly!
The previous version of this patch I submitted used exactly that, but then for some reason I thought people could find std::min a bit odd here, and so I changed it.
But now I think I was right the first time around :)


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3140
+/// \param ReturnType This is the return type of the function.
+void Sema::updNRVOResultWithRetType(NRVOResult &Res, QualType ReturnType) {
+  if (!Res.Candidate)
----------------
aaronpuchert wrote:
> `NRVOResult` seems to be small, why not make this a proper function and let it return the result?
It does use the result parameter as input and output. This function can only "downgrade" a result it received previously.
I could make it receive a result and return the result, but then would probably just use it like this:
```
NRVORes = updNRVOResultWithRetType(NRVORes, ...);
```
Do you think that looks clearer?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696