LGTM with a couple of nits. I like this change!
http://codereview.chromium.org/7227010/diff/4001/src/code-stubs.cc File src/code-stubs.cc (right): http://codereview.chromium.org/7227010/diff/4001/src/code-stubs.cc#newcode249 src/code-stubs.cc:249: case JSObject::FAST_ELEMENTS: nit: indent "case" lines inside "switch" statements. http://codereview.chromium.org/7227010/diff/4001/src/code-stubs.cc#newcode278 src/code-stubs.cc:278: case JSObject::FAST_ELEMENTS: Same indentation nit here. http://codereview.chromium.org/7227010/diff/4001/src/code-stubs.h File src/code-stubs.h (right): http://codereview.chromium.org/7227010/diff/4001/src/code-stubs.h#newcode953 src/code-stubs.h:953: explicit KeyedStoreElementStub(bool is_js_array, "explicit" isn't needed here. http://codereview.chromium.org/7227010/diff/4001/src/ic.cc File src/ic.cc (right): http://codereview.chromium.org/7227010/diff/4001/src/ic.cc#newcode1693 src/ic.cc:1693: Code* generic_stub) { I think you no longer need |generic_stub|. http://codereview.chromium.org/7227010/diff/4001/src/stub-cache.h File src/stub-cache.h (right): http://codereview.chromium.org/7227010/diff/4001/src/stub-cache.h#newcode665 src/stub-cache.h:665: static void GenerateLoadDictionaryElement(MacroAssembler* masmx); x? http://codereview.chromium.org/7227010/diff/4001/test/mjsunit/polymorph-arrays.js File test/mjsunit/polymorph-arrays.js (right): http://codereview.chromium.org/7227010/diff/4001/test/mjsunit/polymorph-arrays.js#newcode29 test/mjsunit/polymorph-arrays.js:29: // Flags: --allow-natives-syntax --expose-gc AFAICS you're not calling GC anywhere, so the first "// Flags:" line should be enough. http://codereview.chromium.org/7227010/diff/4001/test/mjsunit/polymorph-arrays.js#newcode80 test/mjsunit/polymorph-arrays.js:80: assertEquals(1, load(object_array, 1)); Since exactly this assertion is performed inside make_polymorphic_load_function() as well, I think it's unnecessary to do it again here. The same is true for the next few lines, so I guess this whole paragraph could be removed without loss of test coverage. But I don't feel strongly about this, feel free to leave it as is if you prefer the explicitness of it. http://codereview.chromium.org/7227010/diff/4001/test/mjsunit/polymorph-arrays.js#newcode108 test/mjsunit/polymorph-arrays.js:108: assertEquals(undefined, load(js_array, new Object())); Duplicate line. http://codereview.chromium.org/7227010/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
