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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
