Issue 2832001: Add support for elements and array indices in Object.defineProperty...

Issue 2832001: Add support for elements and array indices in Object.defineProperty... (Closed)

Created:
10 years, 6 months ago by Rico

Modified:
9 years, 6 months ago

Reviewers:
Mads Ager (chromium)

CC:
v8-dev

Base URL:
http://v8.googlecode.com/svn/branches/bleeding_edge/

Visibility:
Public.

Description

Add support for elements and array indices in Object.defineProperty (fixes bug 619). This also fixes a bug in GetOwnProperty in runtime.cc discovered by the new test cases. That part of the code was not testable before since we had no way of correctly defining properties on elements. Committed: http://code.google.com/p/v8/source/detail?r=4862

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 10

Patch Set 3 : '' #

Created: 10 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+183 lines, -69 lines)			Patch
M	src/objects.h	View		1 chunk	+5 lines, -0 lines	0 comments	Download
M	src/runtime.cc	View	1 2	3 chunks	+23 lines, -4 lines	0 comments	Download
D	test/mjsunit/bugs/bug-619.js	View		1 chunk	+0 lines, -62 lines	0 comments	Download
M	test/mjsunit/object-define-property.js	View	1 2	1 chunk	+153 lines, -0 lines	0 comments	Download
A +	test/mjsunit/regress/regress-619.js	View		1 chunk	+2 lines, -3 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Mads Ager (chromium)

LGTM http://codereview.chromium.org/2832001/diff/4001/5002 File src/runtime.cc (right): http://codereview.chromium.org/2832001/diff/4001/5002#newcode3873 src/runtime.cc:3873: // Check if this is an element. Could ...

10 years, 6 months ago (2010-06-14 11:57:41 UTC) #2

Rico

10 years, 6 months ago (2010-06-14 13:59:37 UTC) #3

http://codereview.chromium.org/2832001/diff/4001/5002
File src/runtime.cc (right):

http://codereview.chromium.org/2832001/diff/4001/5002#newcode3873
src/runtime.cc:3873: // Check if this is an element.
On 2010/06/14 11:57:41, Mads Ager wrote:
> Could we move this check earlier? We should only search for the property as a
> named property if it is not an array index.

Done.

http://codereview.chromium.org/2832001/diff/4001/5002#newcode3880
src/runtime.cc:3880: if ((unchecked & (DONT_DELETE | DONT_ENUM | READ_ONLY)) &&
On 2010/06/14 11:57:41, Mads Ager wrote:
> Explictly check not equal to 0?

Done.

http://codereview.chromium.org/2832001/diff/4001/5004
File test/mjsunit/object-define-property.js (right):

http://codereview.chromium.org/2832001/diff/4001/5004#newcode726
test/mjsunit/object-define-property.js:726: var descElement = {value: 'foobar'};
On 2010/06/14 11:57:41, Mads Ager wrote:
> Please add a bit more spacing for these literals:
> 
> { value: 'foobar' }

Done.

http://codereview.chromium.org/2832001/diff/4001/5004#newcode783
test/mjsunit/object-define-property.js:783: // Define non existing property -
all attributes should default to false
On 2010/06/14 11:57:41, Mads Ager wrote:
> Period at end of comment.

Done.

http://codereview.chromium.org/2832001/diff/4001/5004#newcode804
test/mjsunit/object-define-property.js:804: var descElement = {value: 'foobar'};
On 2010/06/14 11:57:41, Mads Ager wrote:
> A bit of spacing.

Done.

Expand Messages | Collapse Messages