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

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

Created:
5 years, 3 months ago by Peter Varga
Modified:
5 years, 3 months 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
Trybot results:

Messages

Total messages: 8 (0 generated)
Peter Varga
5 years, 3 months 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 ...
5 years, 3 months ago (2012-10-10 17:10:10 UTC) #2
Michael Starzinger
LGTM. I'll land this for you. Thanks for the contribution!
5 years, 3 months 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 ...
5 years, 3 months 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 ...
5 years, 3 months 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]]: ...
5 years, 3 months 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: ...
5 years, 3 months ago (2012-10-15 15:30:18 UTC) #7
Peter Varga
5 years, 3 months 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 0eb02b776