Ensure correct enumeration indices in the dict

Ensure correct enumeration indices in the dict


BUG=chromium:148376
Committed: https://code.google.com/p/v8/source/detail?r=12494

[Toon Verwaest, 2012-09-12 16:23:13]

PTAL.

[Michael Starzinger, 2012-09-12 17:06:55]

https://chromiumcodereview.appspot.com/10908216/diff/1/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/10908216/diff/1/src/objects.cc#newcode...
src/objects.cc:9405: if (details.dictionary_index() == 0 && !(attributes &
DONT_ENUM)) {
Why do we check for DONT_ENUM here? Do we do that anywhere else in the code? If
not I would just allocate enumeration indices. If we change the DONT_ENUM
attribute after the fact, the enumeration order will be screwed up because we
didn't hand out an index when the property was defined.

However I am not sure if my comment even applies to elements as opposed to
properties.

Also I don't understand why we check for dictionary_index == 0 instead of
IsDeleted() here. Shouldn't the invariant be that every non-deleted property has
a valid enumeration index?

https://chromiumcodereview.appspot.com/10908216/diff/1/test/mjsunit/regress/r...
File test/mjsunit/regress/regress-crbug-148376.js (right):

https://chromiumcodereview.appspot.com/10908216/diff/1/test/mjsunit/regress/r...
test/mjsunit/regress/regress-crbug-148376.js:28: // Flags:
--allow-natives-syntax
We don't need natives syntax here.

[Toon Verwaest, 2012-09-13 08:46:32]

Removed the elements-related code. Also removed the ASSERT from DetailsAtPut
since it should not trigger for elements. Since we do not have the key in
DetailsAtPut we can't distinguish between elements and properties.

https://chromiumcodereview.appspot.com/10908216/diff/1/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/10908216/diff/1/src/objects.cc#newcode...
src/objects.cc:9405: if (details.dictionary_index() == 0 && !(attributes &
DONT_ENUM)) {
You are right, this code is unnecessary for elements. It just invalidly
triggered the ASSERT in DetailsAtPut, which is now removed. We only have to
ensure enumeration indices for properties.

On 2012/09/12 17:06:55, Michael Starzinger wrote:
> Why do we check for DONT_ENUM here? Do we do that anywhere else in the code?
If
> not I would just allocate enumeration indices. If we change the DONT_ENUM
> attribute after the fact, the enumeration order will be screwed up because we
> didn't hand out an index when the property was defined.
> 
> However I am not sure if my comment even applies to elements as opposed to
> properties.
> 
> Also I don't understand why we check for dictionary_index == 0 instead of
> IsDeleted() here. Shouldn't the invariant be that every non-deleted property
has
> a valid enumeration index?

[Michael Starzinger, 2012-09-13 08:51:17]

LGTM.

https://chromiumcodereview.appspot.com/10908216/diff/2002/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/10908216/diff/2002/src/objects.cc#newc...
src/objects.cc:499: ASSERT(enumeration_index != 0);
We could assert for enumeration_index > 0 here. Just a nit, feel free to ignore.