Chromium Code Reviews
Help | Chromium Project | Sign in
(166)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 5 months ago by rossberg
Modified:
1 year, 5 months ago
Reviewers:
Michael Starzinger
CC:
v8-dev_googlegroups.com
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) Lint Patch
M src/objects.h View 1 2 3 2 chunks +3 lines, -20 lines 0 comments 0 errors Download
M src/objects.cc View 1 2 3 4 chunks +25 lines, -105 lines 0 comments 0 errors Download
M src/objects-inl.h View 1 chunk +4 lines, -0 lines 0 comments 0 errors Download
M src/runtime.cc View 1 2 3 chunks +87 lines, -218 lines 0 comments 0 errors Download
M test/mjsunit/regress/regress-1692.js View 1 chunk +1 line, -1 line 0 comments ? errors Download
Trybot results:
Commit:

Messages

Total messages: 4
rossberg
1 year, 5 months ago #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 ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #3
Michael Starzinger
1 year, 5 months ago #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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6