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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Didn't really check for correctness yet, just a superficial review.

I like the idea, splitting the functionality up definitely helps understanding this.



================
Comment at: clang/include/clang/Sema/Sema.h:4733
+
+    bool isMoveEligible() const { return S >= MoveEligible; };
+    bool isCopyElidable() const { return S == MoveEligibleAndCopyElidable; }
----------------
Nitpick: I'd go with `!= None` here.


================
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())
----------------
Perhaps this should just be direct initialization? Can't really find anything in the standard though.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:854
   if (Ex && !Ex->isTypeDependent()) {
+    NRVOResult NRVORes = IsThrownVarInScope ? getNRVOResult(Ex) : NRVOResult();
+
----------------
Any reason why you've moved that away from the comment that wants to explain it?


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3030-3032
+  Res.S = Res.S != Sema::NRVOResult::None && CanMove
+              ? Sema::NRVOResult::MoveEligible
+              : Sema::NRVOResult::None;
----------------
The way you've designed the enumeration, couldn't you use `std::min` here?


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3040
 ///
-/// \param E The expression being returned from the function or block, or
-/// being thrown.
+/// \\param Parenthesized Whether the expression this candidate was obtained
+/// from was parenthesized.
----------------
The second backslash seems superfluous.


================
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) {
----------------
These comments seem to be separated from their context now...


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3059
+  // (other than a function ... parameter)
+  if (VD->getKind() == Decl::ParmVar) {
+    downgradeNRVOResult(Res, hasCXX11);
----------------
Braces are usually omitted if both branches are single statements.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3073
 
   // Return false if VD is a __block variable. We don't want to implicitly move
   // out of a __block variable during a return because we cannot assume the
----------------
Just remove "Return false if VD is a __block variable." No need to repeat the code, what follows is important.


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3107
+
+/// Determine whether the given expression might be move-eligible or
+/// copy-elidable in either a (co_)return statement or throw expression,
----------------
Can we maybe move this function up one place to make the diff a bit smaller? It appears to contain the first part of the original `getCopyElisionCandidate`.


================
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)
----------------
`NRVOResult` seems to be small, why not make this a proper function and let it return the result?


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3156
+      // If is decltype(auto) and the original return expression
+      // was parethesized, then we can discard this candidate because
+      // it would deduce to a reference.
----------------
parenthesized


================
Comment at: clang/lib/Sema/SemaStmt.cpp:3157-3159
+      // it would deduce to a reference.
+      if (AT->isDecltypeAuto() && Res.isParenthesized)
+        return Res = NRVOResult(), void();
----------------
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.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1062
+    else
+      assert(false && "Unknown context type");
+
----------------
`llvm_unreachable`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99696