Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(62)

Issue 11094021: Fix bug in deletion of indexed properties (Closed)

Created:
6 years, 1 month ago by Peter Varga
Modified:
6 years, 1 month ago
Reviewers:
Michael Starzinger
CC:
v8-dev
Visibility:
Public.

Description

Fix bug in deletion of indexed properties The delete operator always return true in case of indexed property. It should return false if an indexed property can't be deleted (eg. DontDelete attribute is set or a string object is the holder). Contributed by Peter Varga <pvarga@inf.u-szeged.hu>; BUG=none TEST=mjsunit/delete-non-configurable Committed: https://code.google.com/p/v8/source/detail?r=12736

Patch Set 1 #

Patch Set 2 : Replace api test by mjsunit test #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+77 lines, -33 lines) Patch
M src/elements.cc View 1 1 chunk +21 lines, -22 lines 0 comments Download
M src/objects.cc View 1 1 chunk +15 lines, -0 lines 4 comments Download
A + test/mjsunit/delete-non-configurable.js View 1 1 chunk +41 lines, -11 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
Peter Varga
6 years, 1 month ago (2012-10-09 11:04:47 UTC) #1
Michael Starzinger
The CL generally looks good and also the test coverage is fine. But I think ...
6 years, 1 month ago (2012-10-10 17:10:10 UTC) #2
Michael Starzinger
LGTM. I'll land this for you. Thanks for the contribution!
6 years, 1 month ago (2012-10-15 15:09:33 UTC) #3
Michael Starzinger
https://codereview.chromium.org/11094021/diff/3001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/11094021/diff/3001/src/objects.cc#newcode3946 src/objects.cc:3946: if (IsStringObjectWithCharacterAt(index)) { This was moved from DeleteElement() to ...
6 years, 1 month ago (2012-10-15 15:20:39 UTC) #4
Peter Varga
https://codereview.chromium.org/11094021/diff/3001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/11094021/diff/3001/src/objects.cc#newcode3946 src/objects.cc:3946: if (IsStringObjectWithCharacterAt(index)) { This is on purpose. Based on ...
6 years, 1 month ago (2012-10-15 15:25:34 UTC) #5
Peter Varga
See section 15.5.5.1 "It (String object) has the attributes { [[Writable]]: false, [[Enumerable]]: false, [[Configurable]]: ...
6 years, 1 month ago (2012-10-15 15:30:12 UTC) #6
Michael Starzinger
https://codereview.chromium.org/11094021/diff/3001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/11094021/diff/3001/src/objects.cc#newcode3946 src/objects.cc:3946: if (IsStringObjectWithCharacterAt(index)) { On 2012/10/15 15:25:34, Peter Varga wrote: ...
6 years, 1 month ago (2012-10-15 15:30:18 UTC) #7
Peter Varga
6 years, 1 month ago (2012-10-15 15:48:23 UTC) #8
https://codereview.chromium.org/11094021/diff/3001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11094021/diff/3001/src/objects.cc#newcode3946
src/objects.cc:3946: if (IsStringObjectWithCharacterAt(index)) {
I see the problem now. It seems git applied the patch wrongly.

Powered by Google App Engine
This is Rietveld 408576698