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 

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 


cfe-commits mailing list

Reply via email to