[PATCH] D43898: Preliminary refactoring in service of -Wreturn-std-move. NFC.

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone marked 4 inline comments as done.
Quuxplusone added inline comments.


================
Comment at: lib/Sema/SemaStmt.cpp:2970
+      FunctionDecl *FD = Step.Function.Function;
+      if (isa<CXXConstructorDecl>(FD)) {
+        // C++14 [class.copy]p32:
----------------
rtrieu wrote:
> Use early exit here:
> 
> 
> ```
> if (!isa<CXXConstructorDecl>(FD)
>   continue;
> 
> // old if-block code
> ```
I'd prefer not to do this, since D43322 is going to change this code into "if isa-red-fish... else if isa-blue-fish...". Therefore I think it makes sense to keep this intermediate stage as "if isa-red-fish...", rather than changing it into "if not-isa-red-fish continue... otherwise..."

If you really want this, I can change it; but it's just going to change back in D43322, and the goal of this patch was to make D43322 smaller.


================
Comment at: lib/Sema/SemaStmt.cpp:2999-3000
-        // expression node to persist.
-        Value = ImplicitCastExpr::Create(Context, Value->getType(), CK_NoOp,
-                                         Value, nullptr, VK_XValue);
 
----------------
rtrieu wrote:
> At this point, the variable Value is updated.  Value is scoped to this function, and used again after this code.  In your change, there is a new Value variable in the static function.  Only that variable is updated and not this one, making this a change in behavior.
Good catch! I've addressed this now by making the parameter `Expr *&Value`; but I'd be open to better approaches. Particularly because I still don't know what to do about the "unnecessary promoting `Value` to the heap" that will happen in D43322.


Repository:
  rC Clang

https://reviews.llvm.org/D43898