rjmccall added a comment.
I'm sorry, I did a review of your patch on Phabricator but apparently never
submitted it.
================
Comment at: lib/CodeGen/CGCall.h:219
+ RValue RV;
+ LValue LV; /// The l-value from which the argument is derived.
+ };
----------------
This could be clearer. I would say something like "The argument is
semantically a load from this l-value."
================
Comment at: lib/CodeGen/CGCall.h:221
+ };
+ bool HasLV;
+
----------------
Please add an IsUsed flag so that you can assert that e.g. getRValue and/or
copyInto aren't called twice. It's okay for this to be `mutable`, since it's
just a data-flow assertion rather than logically a change to the value of the
argument.
================
Comment at: lib/CodeGen/CGCall.h:226
+ CallArg(RValue rv, QualType ty) : RV(rv), HasLV(false), Ty(ty) {}
+ CallArg(LValue _LV, QualType ty) : LV(_LV), HasLV(true), Ty(ty) {}
+ bool hasLValue() const { return HasLV; }
----------------
Why the two different naming conventions for these parameters? Just call the
new one `lv` for consistency, please.
================
Comment at: lib/CodeGen/CGCall.h:234
+
+ LValue getLValue() const {
+ assert(HasLV);
----------------
Please name this something like `getKnownLValue` and add a parallel
`getKnownRValue`. These should assert `!IsUsed` but not set it.
================
Comment at: lib/CodeGen/CGCall.h:248
+ return HasLV ? LV.getAddress() : RV.getAggregateAddress();
+ }
+
----------------
Part of my thinking in suggesting this representation change was that the
current representation was prone to a certain kind of bug where clients blindly
use the RValue without realizing that it's actually something they need to copy
from instead of using directly. That is generally a bug because indirect
arguments are expected to be independent slots and are not permitted to alias.
The new representation implicitly fixes these bugs by pushing users towards
using one of these approaches.
All of this is to say that I'm not thrilled about having a getAggregateAddress
here.
https://reviews.llvm.org/D34367
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
