Implemented KeyedCallIC, incorporated CR feedback (except for marking all
builtins strict - will investigate this part later), and adding more tests
to
cover keyed call ICs and using strict/non-strict getters.
Thank you!
Martin
http://codereview.chromium.org/6523052/diff/2001/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):
http://codereview.chromium.org/6523052/diff/2001/src/arm/stub-cache-arm.cc#newcode2335
src/arm/stub-cache-arm.cc:2335: if (!function->IsBuiltin() &&
!function_info->strict_mode()) {
On 2011/02/16 13:34:30, Lasse Reichstein wrote:
It would be great if we could just make all builtin functions strict,
and drop
the IsBuiltin() check.
Good idea, Lasse. If it is ok, I'll put this on my todo list rather than
include here. I looked for some place where all builtin functions are
going through when they are materialized (all of C, Asm and JS builtins)
but am still looking for the right spot where to flip the bit on the
function's shared info.
http://codereview.chromium.org/6523052/diff/2001/src/ic.cc
File src/ic.cc (right):
http://codereview.chromium.org/6523052/diff/2001/src/ic.cc#newcode464
src/ic.cc:464: // TODO(mmaly): One cannot hang number-indexed functions
off of values, right?
On 2011/02/16 10:43:54, Mads Ager wrote:
On 2011/02/16 04:56:17, Martin Maly wrote:
> I couldn't come up with a way where value could actually be valid
here. I may
be
> wrong though...
I think that is because we have a bug here. I think you should leave
this code
as is. GetElement should deal with prototypes for value objects the
same way
that GetProperty does.
The case that we should hit here is:
Number.prototype[0] = function() { print('asdf'); }
(0)["0"]()
We do not hit this and we throw an error. This is inconsistent and we
should
change it. Let's file a bug report and deal with that in a separate
change.
Done.
http://codereview.chromium.org/6523052/diff/2001/src/ic.cc#newcode490
src/ic.cc:490: // TODO(mmaly): Should we do ToObject here?
On 2011/02/16 10:43:54, Mads Ager wrote:
Nope, there is no change to this code and it makes sense to use the
value object
for error reporting. That is what you attempted to call something on.
The ReceiverToObject call only changes the receiver on the stack I
believe, so
you have not made any change to behavior here.
Done.
http://codereview.chromium.org/6523052/diff/2001/src/ic.cc#newcode511
src/ic.cc:511: ReceiverToObject(object);
On 2011/02/16 11:52:12, Vitaly wrote:
1. Since this is now done after UpdateCaches, please fix
TryUpdateExtraICState.
It assumes it's invoked with a wrapped receiver.
2. For builtin functions it would be nice to avoid wrapping the
receiver. It
could be as simple as checking whether the function is a builtin here.
If turns
out to be more involved feel free to ignore this suggestion.
Done and done. for the builtin change I ran full test battery in debug
mode, all clean, so I suppose it was as easy as checking for builtin
function here. Thanks for the suggestion.
http://codereview.chromium.org/6523052/diff/11001/src/ic.cc#newcode437
src/ic.cc:437:
Since the logic now includes checking the function, and it is used in 2
places, I lifted it into renamed ReceiverToobject...
http://codereview.chromium.org/6523052/diff/11001/src/ic.cc#newcode582
src/ic.cc:582: index = Smi::cast(args[1])->value();
Good catch, Vitaly, this assert did trigger in debug mode tests as it
did depend on wrapping.
http://codereview.chromium.org/6523052/diff/11001/src/ic.h
File src/ic.h (right):
http://codereview.chromium.org/6523052/diff/11001/src/ic.h#newcode227
src/ic.h:227: void ReceiverToObjectIfRequired(Object* callee,
Handle<Object> object);
I am very open to suggestions on a better name.
http://codereview.chromium.org/6523052/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
