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

Issue 11420011: Clean-up refactoring to eliminate GetLocalElementKind. (Closed)

Created:
5 years ago by rossberg
Modified:
5 years ago
Reviewers:
Michael Starzinger
CC:
v8-dev
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Oops... #

Patch Set 3 : Eps. #

Total comments: 10

Patch Set 4 : Addressed comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+120 lines, -344 lines) Patch
M src/objects.h View 1 2 3 2 chunks +3 lines, -20 lines 0 comments Download
M src/objects.cc View 1 2 3 4 chunks +25 lines, -105 lines 0 comments Download
M src/objects-inl.h View 1 chunk +4 lines, -0 lines 0 comments Download
M src/runtime.cc View 1 2 3 chunks +87 lines, -218 lines 0 comments Download
M test/mjsunit/regress/regress-1692.js View 1 chunk +1 line, -1 line 0 comments Download
Trybot results:

Messages

Total messages: 4 (0 generated)
rossberg
5 years ago (2012-11-15 18:07:53 UTC) #1
Michael Starzinger
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 ...
5 years ago (2012-11-15 21:45:37 UTC) #2
rossberg
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 ...
5 years ago (2012-11-16 12:50:02 UTC) #3
Michael Starzinger
5 years ago (2012-11-16 13:18:51 UTC) #4
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.

Powered by Google App Engine
This is Rietveld 0eb02b776