Clean-up refactoring to eliminate GetLocalElementKind.

Clean-up refactoring to eliminate GetLocalElementKind.

Eliminates substantial amounts of fragile code duplication and special casing.

Also fixes "a".propertyIsEnumerable(0) to correctly return true.

R=mstarzinger@chromium.org
BUG=

Committed: https://code.google.com/p/v8/source/detail?r=12990

[Michael Starzinger, 2012-11-15 21:45:37]

The comments boil down to one question: "Where should the AsArrayIndex() go?"

https://codereview.chromium.org/11420011/diff/5001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/11420011/diff/5001/src/objects-inl.h#newcode5039
src/objects-inl.h:5039: }
I am not convinced that this is the right place to defer to the element-case if
the key is convertible to an index. I would rather have FooPropertyBar() only
deal with properties and FooElementBar() only deal with elements.

I understand that the callers throughout the codebase should have one entry
point and not care about that distinction. Maybe having
FooPropertyOrElementBar() in between would be a good idea. Now the name
obviously needs some improvement.

WDYT?

https://codereview.chromium.org/11420011/diff/5001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11420011/diff/5001/src/objects.cc#newcode252
src/objects.cc:252: if (name->AsArrayIndex(&index)) return GetElement(object,
index);
See comment in objects-inl.h about that.

With this one I am even more convinced that it's the wrong place. Either the
unhandlified GetProperty() defers or the caller does it. But this handlified
method should just dispatch.

https://codereview.chromium.org/11420011/diff/5001/src/objects.h
File src/objects.h (left):

https://codereview.chromium.org/11420011/diff/5001/src/objects.h#oldcode1853
src/objects.h:1853: enum LocalElementKind {
Oh yeah, LocalElementKind be gone. I like that!

https://codereview.chromium.org/11420011/diff/5001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/11420011/diff/5001/src/objects.h#newcode1850
src/objects.h:1850: // These methods do not perform access checks!
Add empty newline above the comment.

https://codereview.chromium.org/11420011/diff/5001/test/mjsunit/regress/regre...
File test/mjsunit/regress/regress-1692.js (right):

https://codereview.chromium.org/11420011/diff/5001/test/mjsunit/regress/regre...
test/mjsunit/regress/regress-1692.js:85: assertTrue(o.propertyIsEnumerable(0));
Nice catch!

[rossberg, 2012-11-16 12:50:02]

https://codereview.chromium.org/11420011/diff/5001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/11420011/diff/5001/src/objects-inl.h#newcode5039
src/objects-inl.h:5039: }
On 2012/11/15 21:45:37, Michael Starzinger wrote:
> I am not convinced that this is the right place to defer to the element-case
if
> the key is convertible to an index. I would rather have FooPropertyBar() only
> deal with properties and FooElementBar() only deal with elements.
> 
> I understand that the callers throughout the codebase should have one entry
> point and not care about that distinction. Maybe having
> FooPropertyOrElementBar() in between would be a good idea. Now the name
> obviously needs some improvement.
> 
> WDYT?

I share your concern, but we already do this in various places in objects.cc, so
your suggestion would have to be part of a (much) larger (and likely
non-trivial) clean-up.

https://codereview.chromium.org/11420011/diff/5001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11420011/diff/5001/src/objects.cc#newcode252
src/objects.cc:252: if (name->AsArrayIndex(&index)) return GetElement(object,
index);
On 2012/11/15 21:45:37, Michael Starzinger wrote:
> See comment in objects-inl.h about that.
> 
> With this one I am even more convinced that it's the wrong place. Either the
> unhandlified GetProperty() defers or the caller does it. But this handlified
> method should just dispatch.

As discussed offline, I put this here mainly for performance reasons. I agree
it's not nice. Again, a consistent clean-up is a larger project.

Added a TODO.

https://codereview.chromium.org/11420011/diff/5001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/11420011/diff/5001/src/objects.h#newcode1850
src/objects.h:1850: // These methods do not perform access checks!
On 2012/11/15 21:45:37, Michael Starzinger wrote:
> Add empty newline above the comment.

Done.

[Michael Starzinger, 2012-11-16 13:18:51]

LGTM.

https://codereview.chromium.org/11420011/diff/5001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/11420011/diff/5001/src/objects-inl.h#newcode5039
src/objects-inl.h:5039: }
On 2012/11/16 12:50:02, rossberg wrote:
> On 2012/11/15 21:45:37, Michael Starzinger wrote:
> > I am not convinced that this is the right place to defer to the element-case
> if
> > the key is convertible to an index. I would rather have FooPropertyBar()
only
> > deal with properties and FooElementBar() only deal with elements.
> > 
> > I understand that the callers throughout the codebase should have one entry
> > point and not care about that distinction. Maybe having
> > FooPropertyOrElementBar() in between would be a good idea. Now the name
> > obviously needs some improvement.
> > 
> > WDYT?
> 
> I share your concern, but we already do this in various places in objects.cc,
so
> your suggestion would have to be part of a (much) larger (and likely
> non-trivial) clean-up.

Yes, I agree. This doesn't have to be part of this CL as long as we agree that
this particular situation is far from perfect.

https://codereview.chromium.org/11420011/diff/5001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11420011/diff/5001/src/objects.cc#newcode252
src/objects.cc:252: if (name->AsArrayIndex(&index)) return GetElement(object,
index);
On 2012/11/16 12:50:02, rossberg wrote:
> On 2012/11/15 21:45:37, Michael Starzinger wrote:
> > See comment in objects-inl.h about that.
> > 
> > With this one I am even more convinced that it's the wrong place. Either the
> > unhandlified GetProperty() defers or the caller does it. But this handlified
> > method should just dispatch.
> 
> As discussed offline, I put this here mainly for performance reasons. I agree
> it's not nice. Again, a consistent clean-up is a larger project.
> 
> Added a TODO.

Together with the TODO I can live with this. And this cleanup CL is definitely
going into the right direction.