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/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
