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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3030-3032
+  Res.S = Res.S != Sema::NRVOResult::None && CanMove
+              ? Sema::NRVOResult::MoveEligible
+              : Sema::NRVOResult::None;
----------------
mizvekov wrote:
> 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 :)
Perhaps you can just add a comment to the enumeration saying that the values are totally ordered with regards to implication so that ⇒ is >=.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3058
 
-bool Sema::isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD,
-                                  CopyElisionSemanticsKind CESK) {
-  QualType VDType = VD->getType();
-  // - in a return statement in a function with ...
-  // ... a class return type ...
-  if (!ReturnType.isNull() && !ReturnType->isDependentType()) {
-    if (!ReturnType->isRecordType())
-      return false;
-    // ... the same cv-unqualified type as the function return type ...
-    // When considering moving this expression out, allow dissimilar types.
-    if (!(CESK & CES_AllowDifferentTypes) && !VDType->isDependentType() &&
-        !Context.hasSameUnqualifiedType(ReturnType, VDType))
-      return false;
+  // (other than a function ... parameter)
+  if (VD->getKind() == Decl::ParmVar) {
----------------
mizvekov wrote:
> aaronpuchert wrote:
> > These comments seem to be separated from their context now...
> This one comment here is talking about the line just below it.
> Any other examples where it seems separated?
I meant the context of the comment itself. Originally this was quoting a clause

```
  // - in a return statement in a function [where] ...
  // ... the expression is the name of a non-volatile automatic object ...
```

and then additional parts later. Now you're having that initial part in a different function and it's not immediately clear what you're quoting here.

To be clear, this was about "(other than a function ... parameter)" below.


================
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)
----------------
mizvekov wrote:
> 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?
Yes, that would seem more natural to me. Technically a caller could decide to not use the same variable in both places, for example it could pass a temporary or pass the result on directly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696