I didn't look at the arm code. It looks about right. My comments go for
both
platforms.
http://codereview.chromium.org/487017/diff/28004/20015
File src/ia32/codegen-ia32.cc (right):
http://codereview.chromium.org/487017/diff/28004/20015#newcode2357
src/ia32/codegen-ia32.cc:2357: // Frame[3]: applicand.
The period looks funny.
http://codereview.chromium.org/487017/diff/28004/20015#newcode4905
src/ia32/codegen-ia32.cc:4905: Reference ref(this, property, false);
False is the default value, isn't it?
http://codereview.chromium.org/487017/diff/28004/20015#newcode4910
src/ia32/codegen-ia32.cc:4910: Reference ref(this, property, false);
Do you really need all this? If you just cut out all the keyed load
code from Reference::GetValue except the final VirtualFrame::Push, and
you call it GenerateKeyedLoad or EmitKeyedLoad and return the Result,
you can do:
Load(property->obj());
Load(property->key());
Result function = EmitKeyedLoad();
frame()->Drop(); // Key.
Result receiver = frame()->Pop();
frame()->Push(&function);
frame()->Push(&receiver);
http://codereview.chromium.org/487017/diff/28004/20016
File src/ia32/codegen-ia32.h (right):
http://codereview.chromium.org/487017/diff/28004/20016#newcode52
src/ia32/codegen-ia32.h:52: // no dynamic state, and uses zero frame
slots. Note that some variables
There's too much irrelevant detail in this comment.
I don't like the "Note that..." part of this comment because it will be
out of date as soon as we change VariableProxy rewrites, at which point
it will be doing more harm than good.
http://codereview.chromium.org/487017/diff/28004/20016#newcode54
src/ia32/codegen-ia32.h:54: // A property whose name is known at compile
time uses one frame slot,
This part isn't quite right. a["1"] will use two frame slots, for
instance. You can say "A property with key()->IsPropertyName() will use
one...." or some such.
http://codereview.chromium.org/487017/diff/28004/20016#newcode56
src/ia32/codegen-ia32.h:56: // An indexed property, whose name is given
by an expression,
This isn't really true either (in fact, the key is always "given by an
expression" as it has type Expression*).
http://codereview.chromium.org/487017/diff/28004/20016#newcode65
src/ia32/codegen-ia32.h:65: bool is_compound_assignment = false);
You might consider changing this name to something less (today's)
implementation-specific. Knowing us, we'll use the true value for
something other than compound assignments somewhere and it'll just be
confusing.
How about something that has to do with what it means?
"should_preserve" or "is_persistent" or something?
http://codereview.chromium.org/487017/diff/28004/20016#newcode82
src/ia32/codegen-ia32.h:82: return (type_ == ILLEGAL || type_ ==
UNLOADED) ? 0 : type_;
This already relies on the order of enumeration values. You could
write:
return (type < SLOT) ? 0 : type_;
or even
return Max(0, type_);
http://codereview.chromium.org/487017/diff/28004/20016#newcode501
src/ia32/codegen-ia32.h:501: // x.apply(y, arguments). We call x the
applicand and y the receiver.
I wouldn't describe (here) so much detail about how the function is
implemented because that could change and isn't important to the
interface.
You might just indicate that we're trying to avoid allocating the
arguments object in that case.
http://codereview.chromium.org/487017
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
