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#newcode5528
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#newcode6609
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#newcode6612
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#newcode6623
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#newcode6642
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#newcode6652
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#newcode546
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?
http://codereview.chromium.org/7356013/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
