http://codereview.chromium.org/2280007/diff/72002/86010 File src/ia32/ic-ia32.cc (right):
http://codereview.chromium.org/2280007/diff/72002/86010#newcode1080 src/ia32/ic-ia32.cc:1080: __ CmpInstanceType(ebx, FIRST_NONSTRING_TYPE); I believe it was a bug we discussed with you? http://codereview.chromium.org/2280007/diff/72002/86010#newcode1100 src/ia32/ic-ia32.cc:1100: __ bind(&miss); maybe pass miss label as an argument? I think it would make it easier to use this function and looks more v8ish imho. http://codereview.chromium.org/2280007/diff/72002/86010#newcode1200 src/ia32/ic-ia32.cc:1200: __ bind(&miss); ditto http://codereview.chromium.org/2280007/diff/72002/86010#newcode1302 src/ia32/ic-ia32.cc:1302: GenerateMonomorphicCacheProbe(masm, argc, Code::KEYED_CALL_IC); isn't there is a problem if GenerateMonomorphicCacheProbe would jump to miss label? In that case you would take smi_key branch, correct? I think that key should be something like null or undefined to hit this path. I believe (but I would appreciate if you add tests for that) that KeyedLoadIC_Generic could cope with such keys, but maybe it should be made more explicit in the code (e.g. rename the label) http://codereview.chromium.org/2280007/diff/72002/86010#newcode1302 src/ia32/ic-ia32.cc:1302: GenerateMonomorphicCacheProbe(masm, argc, Code::KEYED_CALL_IC); similar note: it doesn't look like we go in miss case if we failed to lookup a stub for the name, correct? http://codereview.chromium.org/2280007/diff/72002/86010#newcode1325 src/ia32/ic-ia32.cc:1325: __ j(below, &miss, not_taken); cache probing does some of these checks, I wonder if we could unify them, but that's probably too much of trouble http://codereview.chromium.org/2280007/diff/72002/86011 File src/ic.cc (right): http://codereview.chromium.org/2280007/diff/72002/86011#newcode605 src/ic.cc:605: } else { not insisting, but I'd rather omit the else-clause as you quite from then-clause quickly anyway http://codereview.chromium.org/2280007/diff/72002/86011#newcode1390 src/ic.cc:1390: // Used from ic_<arch>.cc. Lie! used from ic-<arch>.cc :) http://codereview.chromium.org/2280007/diff/72002/86012 File src/ic.h (right): http://codereview.chromium.org/2280007/diff/72002/86012#newcode217 src/ic.h:217: class CallIC: public CallICBase { I know we already inherit from IC and there is no CallIC/KeyedCallIC specific fields, but still when I see growing hierarchy w/o virtual destructors, I am somewhat uneasy http://codereview.chromium.org/2280007/diff/72002/86013 File src/stub-cache.cc (right): http://codereview.chromium.org/2280007/diff/72002/86013#newcode445 src/stub-cache.cc:445: (kind == Code::CALL_IC ? Logger::type: Logger::KEYED_##type) nit: a space before : (coming from ? : construct) http://codereview.chromium.org/2280007/diff/72002/86029 File test/cctest/test-api.cc (right): http://codereview.chromium.org/2280007/diff/72002/86029#newcode7126 test/cctest/test-api.cc:7126: "proto1 = new Object();" please, rename proto1 to o and o to proto1 to have o -> proto1 -> proto2 chain http://codereview.chromium.org/2280007/diff/72002/86029#newcode7144 test/cctest/test-api.cc:7144: // global object which is between interceptor and constant function' holders. am I missing something, I don't see any override here. Overall, it might be good to add a test case for two paths to miss case: when prototype chain changes from receiver to interceptor holder and from interceptor holder to cached lookup holder---we need to be sure correct name is passed. But yes, it should be the case. http://codereview.chromium.org/2280007/diff/72002/86030 File test/mjsunit/keyed-call-ic.js (right): http://codereview.chromium.org/2280007/diff/72002/86030#newcode158 test/mjsunit/keyed-call-ic.js:158: function testTypeTransitions() { why it's empty? http://codereview.chromium.org/2280007/diff/72002/86030#newcode161 test/mjsunit/keyed-call-ic.js:161: testTypeTransitions(); I think you should add a test for receiver changing its type. http://codereview.chromium.org/2280007/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
