Add support for getOwnPropertyDescriptor on array indices (fixes issue 599)....

Add support for getOwnPropertyDescriptor on array indices (fixes issue 599).

This fix adds support for retriving a property descriptor on elements. The
new version supports both fast and slow case elements. In the fast case
we always default configurable, writable, enumerable to true (we don't have
PropertyDetails for fast elements).

A few new tests are added to get-own-property-descriptor.js, I will
add a lot more to object-define-property when I add support for indices in
Object.defineProperty.



Committed: http://code.google.com/p/v8/source/detail?r=4738

[Rico, 2010-05-26 11:08:52]

Small review.

I will add a bunch of additional tests using getOwnPropertyDescriptor when I
submit the next cl enabling Object.defineProperty for elements.

[Mads Ager (chromium), 2010-05-26 11:23:17]

http://codereview.chromium.org/2278002/diff/7001/8001
File src/runtime.cc (right):

http://codereview.chromium.org/2278002/diff/7001/8001#newcode589
src/runtime.cc:589: GetOwnPropertyImplementation(obj, name, &result);
Maybe do the split before? Start out checking if the name is really an array
index and avoid this call if it is?

http://codereview.chromium.org/2278002/diff/7001/8001#newcode594
src/runtime.cc:594: if (!name->AsArrayIndex(&index) || !obj->HasElement(index))
{
Does HasElement do the right thing? Shouldn't you use HasLocalElement?

http://codereview.chromium.org/2278002/diff/7001/8001#newcode603
src/runtime.cc:603: // Since we already did a check earlier on that we actually
have the
Since we already checked that we have an element there is no need to check if
the entry is valid.

http://codereview.chromium.org/2278002/diff/7001/8001#newcode606
src/runtime.cc:606: elms->set(0, Heap::false_value());
Maybe we should define named constants for 0-4. Would make it easier to read the
code for property descriptors.

[Rico, 2010-05-26 13:43:57]

In addition to addressing the comments I have added support for strings (which
have a special case - see ECMAScript 5 15.5.5.2).

http://codereview.chromium.org/2278002/diff/7001/8001
File src/runtime.cc (right):

http://codereview.chromium.org/2278002/diff/7001/8001#newcode589
src/runtime.cc:589: GetOwnPropertyImplementation(obj, name, &result);
On 2010/05/26 11:23:18, Mads Ager wrote:
> Maybe do the split before? Start out checking if the name is really an array
> index and avoid this call if it is?

Done.

http://codereview.chromium.org/2278002/diff/7001/8001#newcode594
src/runtime.cc:594: if (!name->AsArrayIndex(&index) || !obj->HasElement(index))
{
On 2010/05/26 11:23:18, Mads Ager wrote:
> Does HasElement do the right thing? Shouldn't you use HasLocalElement? 

Done.

http://codereview.chromium.org/2278002/diff/7001/8001#newcode603
src/runtime.cc:603: // Since we already did a check earlier on that we actually
have the
On 2010/05/26 11:23:18, Mads Ager wrote:
> Since we already checked that we have an element there is no need to check if
> the entry is valid.
Removed since the changed order makes this clear from the code.

http://codereview.chromium.org/2278002/diff/7001/8001#newcode606
src/runtime.cc:606: elms->set(0, Heap::false_value());
On 2010/05/26 11:23:18, Mads Ager wrote:
> Maybe we should define named constants for 0-4. Would make it easier to read
the
> code for property descriptors.
They problem is that they represent different things depending on if it is a
data property or an accessor as described by the method comment. I could make
two enums or one with mixed names (i.e.,  GET_FUNCTION_OR_VALUE and
SET_FUNCTION_OR_WRITABLE). We would still, however, have magic numbers in
v8natives.js.

[Mads Ager (chromium), 2010-05-26 13:55:12]

LGTM

http://codereview.chromium.org/2278002/diff/7001/8001
File src/runtime.cc (right):

http://codereview.chromium.org/2278002/diff/7001/8001#newcode606
src/runtime.cc:606: elms->set(0, Heap::false_value());
On 2010/05/26 13:43:57, Rico wrote:
> On 2010/05/26 11:23:18, Mads Ager wrote:
> > Maybe we should define named constants for 0-4. Would make it easier to read
> the
> > code for property descriptors.
> They problem is that they represent different things depending on if it is a
> data property or an accessor as described by the method comment. I could make
> two enums or one with mixed names (i.e.,  GET_FUNCTION_OR_VALUE and
> SET_FUNCTION_OR_WRITABLE). We would still, however, have magic numbers in
> v8natives.js.

More than one enum would be fine by me. Also, you can use macros.py to avoid
magic numbers in v8natives.js. Feel free to do that as a followup change though,
but it would be nice for readability.

http://codereview.chromium.org/2278002/diff/8003/12001#newcode597
src/runtime.cc:597: 
This is by -> This is handled by?

http://codereview.chromium.org/2278002/diff/8003/12002
File test/mjsunit/get-own-property-descriptor.js (right):

http://codereview.chromium.org/2278002/diff/8003/12002#newcode33
test/mjsunit/get-own-property-descriptor.js:33: function get() {return x;}
Could you add a bit of spacing here? { return x; }

http://codereview.chromium.org/2278002/diff/14001/15002#newcode28
test/mjsunit/get-own-property-descriptor.js:28: // This file only tests very
simple descriptors that always have
Could you add testing of objects that have the element in their prototype chain
instead of on the object itself? There was a code change to use HasLocalElement
instead of HasElement. We should make sure there is a regression test for that.

[Rico, 2010-05-27 07:26:50]

Mads,
Could you have a final look.
I added index names to both runtime.cc and macros.py (using one enum in
runtime). I also changed v8natives.js to use the new indices.
In addition, I updated es5conform.status to reflect that we now support element
indices.

http://codereview.chromium.org/2278002/diff/7001/8001
File src/runtime.cc (right):

http://codereview.chromium.org/2278002/diff/7001/8001#newcode606
src/runtime.cc:606: elms->set(0, Heap::false_value());
On 2010/05/26 13:55:12, Mads Ager wrote:
> On 2010/05/26 13:43:57, Rico wrote:
> > On 2010/05/26 11:23:18, Mads Ager wrote:
> > > Maybe we should define named constants for 0-4. Would make it easier to
read
> > the
> > > code for property descriptors.
> > They problem is that they represent different things depending on if it is a
> > data property or an accessor as described by the method comment. I could
make
> > two enums or one with mixed names (i.e.,  GET_FUNCTION_OR_VALUE and
> > SET_FUNCTION_OR_WRITABLE). We would still, however, have magic numbers in
> > v8natives.js.
> 
> More than one enum would be fine by me. Also, you can use macros.py to avoid
> magic numbers in v8natives.js. Feel free to do that as a followup change
though,
> but it would be nice for readability.

I changed this to one enum - to be better aligned with what a propertydescriptor
actually is. This changes the length of the returned array but I think this is a
small penalty to pay for the increased readability. I added the indices to
macros.py as well and changed v8natives to use these.

http://codereview.chromium.org/2278002/diff/8003/12001#newcode597
src/runtime.cc:597: 
On 2010/05/26 13:55:12, Mads Ager wrote:
> This is by -> This is handled by?

Done.

http://codereview.chromium.org/2278002/diff/8003/12002
File test/mjsunit/get-own-property-descriptor.js (right):

http://codereview.chromium.org/2278002/diff/8003/12002#newcode33
test/mjsunit/get-own-property-descriptor.js:33: function get() {return x;}
On 2010/05/26 13:55:12, Mads Ager wrote:
> Could you add a bit of spacing here? { return x; }

Done.

http://codereview.chromium.org/2278002/diff/14001/15002#newcode28
test/mjsunit/get-own-property-descriptor.js:28: // This file only tests very
simple descriptors that always have
On 2010/05/26 13:55:12, Mads Ager wrote:
> Could you add testing of objects that have the element in their prototype
chain
> instead of on the object itself? There was a code change to use
HasLocalElement
> instead of HasElement. We should make sure there is a regression test for
that.

Done.

[Mads Ager (chromium), 2010-05-27 07:36:14]

LGTM, much more readable!

http://codereview.chromium.org/2278002/diff/17001/3004
File src/runtime.cc (right):

http://codereview.chromium.org/2278002/diff/17001/3004#newcode594
src/runtime.cc:594: Handle<FixedArray> elms = 
Factory::NewFixedArray(DESCRIPTOR_SIZE);
Remove extra space before Factory::...

http://codereview.chromium.org/2278002/diff/17001/3005
File src/v8natives.js (right):

http://codereview.chromium.org/2278002/diff/17001/3005#newcode495
src/v8natives.js:495: // GetOwnProperty returns and array indexed by the
constants
and -> an