Clean-up refactoring to eliminate GetLocalElementKind. (issue 11420011)

[ross...@chromium.org]

Reviewers: Michael Starzinger,

Description:
Clean-up refactoring to eliminate GetLocalElementKind.

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

R=mstar...@chromium.org
BUG=


Please review this at https://codereview.chromium.org/11420011/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M src/objects-inl.h
M src/objects.h
M src/objects.cc
M src/runtime.cc
M test/mjsunit/regress/regress-1692.js

[mstar...@chromium.org]

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/regress-1692.js
File test/mjsunit/regress/regress-1692.js (right):

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

https://codereview.chromium.org/11420011/

[ross...@chromium.org]

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.

https://codereview.chromium.org/11420011/

[mstar...@chromium.org]

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.

https://codereview.chromium.org/11420011/