Implement Object.prototype.hasOwnProperty in generated code.

Implement Object.prototype.hasOwnProperty in generated code.


R=sgjesse@chromium.org
TEST=mjsunit/has-own-property

[Vitaly Repeshko, 2011-07-13 13:51:07]

Søren,

Can you please take a look?


Thanks,
Vitaly

[Søren Thygesen Gjesse, 2011-07-13 14:00:50]

Redirecting to Daniel as I will be on vacation from tomorrow.

[Mads Ager (chromium), 2011-07-15 09:04:45]

Overall the code looks fine. I'm a bit concerned about the amount of generated
code just for hasOwnProperty. Do we have indications that it is used enough to
justify this much complexity?

It is not completely clear to me whether all cases are covered by your tests. We
should create some API tests as well for the behavior when iterceptors and
access checks are involved.

http://codereview.chromium.org/7356013/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/7356013/diff/1/src/hydrogen.cc#newcode6150
src/hydrogen.cc:6150: HCallStub* result = new(zone()) HCallStub(
I would prefer

HCallStub* result =
    new(zone()) HCallStub(context, CodeStub::HasOwnProperty, 2);

Which also fits the style in the methods above.

http://codereview.chromium.org/7356013/diff/1/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/7356013/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:5528: __ shr(scratch, 6);
This looks wrong? The >> operator in C++ is arithmetic so you might end up with
inconsistent hash values computed in C++ and in generated code which will be
bad. Same thing for the methods below. Maybe for some reason it is OK to use shr
here. I don't think we should - we should make sure that this matches the C++
code.

http://codereview.chromium.org/7356013/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:6609: // TODO(vitalyr): access check?
Yes, I'm pretty sure you need to exclude objects that require access checks as
well. If you should not have access to the object you should not be able to ask
is about which properties it has.

http://codereview.chromium.org/7356013/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:6612: __ CmpInstanceType(map, JS_OBJECT_TYPE);
Do you want to be that restrictive? You don't want this to work for arrays and
global objects?

http://codereview.chromium.org/7356013/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:6623: __ test_b(FieldOperand(scratch1,
Map::kBitFieldOffset),
You are disregarding objects with hidden prototypes because the prototype in
that case is considered part of the object, right? Can you add a comment about
that?

http://codereview.chromium.org/7356013/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:6642: 
Add an abort here so it is clear that there is no fall-through?

http://codereview.chromium.org/7356013/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:6652: 
Ditto, add an abort?

http://codereview.chromium.org/7356013/diff/1/src/ia32/code-stubs-ia32.h
File src/ia32/code-stubs-ia32.h (right):

http://codereview.chromium.org/7356013/diff/1/src/ia32/code-stubs-ia32.h#newc...
src/ia32/code-stubs-ia32.h:546: // returns true if the property is found, jumps
to the given label
the given label -> the not_found_in_descriptors label?