Complete, and finally done. Please suggest any changes you want, or sign
off on
it. Whew.
http://codereview.chromium.org/487017/diff/18001/18002
File src/x64/codegen-x64.cc (right):
http://codereview.chromium.org/487017/diff/18001/18002#newcode664
src/x64/codegen-x64.cc:664: Reference ref(this, apply);
On 2010/01/13 10:40:28, Kevin Millikin wrote:
I still think you should get rid of this use of Reference. There is
no reason
for it. Pass the property's object expression to CallApplyLazy
instead of the
property itself (you've already established that the key is the
literal string
"apply"), and you can write (for Expression* object):
Load(object);
Handle<String> name = Factory::LookupAsciiSymbol("apply");
frame()->Push(name);
Result answer = frame()->CallLoadIC(RelocInfo::CODE_TARGET);
__ nop();
frame()->Push(&answer);
And you've evaluated the object and looked up its apply property.
No need for Dup and shuffling the extra copy of the object out from
under the
result (implicitly via the Reference and GetValue).
I wouldn't worry about optimizing for an inobject property load in
this case,
but if you are, you can abstract out the get-named-property code from
Reference::GetValue (it should be lifted out anyway) and call that.
Done.
http://codereview.chromium.org/487017/diff/18001/18002#newcode688
src/x64/codegen-x64.cc:688: Result probe = frame_->Pop();
On 2010/01/13 10:40:28, Kevin Millikin wrote:
I would just spill the frame here right after the Pop, and leave the
entire rest
of the function spilled.
Done.
http://codereview.chromium.org/487017/diff/18001/18002#newcode785
src/x64/codegen-x64.cc:785: if (try_lazy) { // Separate if (try_lazy)
body, without spilled scope.
On 2010/01/13 10:40:28, Kevin Millikin wrote:
I would eliminate this second test just to get out of the scope. Use
a block
instead.
Done.
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
On 2010/01/14 16:50:55, Kevin Millikin wrote:
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.
Done.
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,
On 2010/01/14 16:50:55, Kevin Millikin wrote:
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.
Done.
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,
On 2010/01/14 16:50:55, Kevin Millikin wrote:
This isn't really true either (in fact, the key is always "given by an
expression" as it has type Expression*).
Done.
http://codereview.chromium.org/487017/diff/28004/20016#newcode65
src/ia32/codegen-ia32.h:65: bool is_compound_assignment = false);
On 2010/01/14 16:50:55, Kevin Millikin wrote:
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?
Changed to "persist_after_get".
http://codereview.chromium.org/487017/diff/28004/20016#newcode82
src/ia32/codegen-ia32.h:82: return (type_ == ILLEGAL || type_ ==
UNLOADED) ? 0 : type_;
On 2010/01/14 16:50:55, Kevin Millikin wrote:
This already relies on the order of enumeration values. You could
write:
return (type < SLOT) ? 0 : type_;
or even
return Max(0, type_);
Done.
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.
On 2010/01/14 16:50:55, Kevin Millikin wrote:
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.
Done.
http://codereview.chromium.org/487017
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
