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/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
The comments boil down to one question: "Where should the AsArrayIndex()
go?"
- [v8-dev] Re: Clean-up refactoring to eliminate GetLocalElement... mstarzinger
