LGTM if comments below are addressed.
http://codereview.chromium.org/8352003/diff/1/src/ic.cc File src/ic.cc (right): http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1131 src/ic.cc:1131: isnan(HeapNumber::cast(*key)->value())) { I slightly prefer Handle<HeapNumber>::cast(key)->value() in handle code. http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1153 src/ic.cc:1153: Handle<Code> code = You might consider indenting these lines like so: Handle<Code> code = isolate()->stub_cache()->ComputeKeyedLoadStringLength( name, string); Because (a) it seems to work better for long function names, and so (b) it can be uniform, and (c) has the advantage of not wasting so vertical space for functions with more than two arguments. Then you could also remove the extra variable and write: Handle<Code> code = isolate()->stub_cache()->ComputeKeyedLoadStringLength( name, Handle<String>::cast(object)); http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1169 src/ic.cc:1169: set_target(*code); On 2011/10/19 13:47:47, ulan wrote:
Add CHECK(!code.is_null()) here and below.
OK but ASSERT, not CHECK. http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1173 src/ic.cc:1173: return Handle<JSArray>::cast(object)->length(); Is this just return array->length()? http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1179 src/ic.cc:1179: JSFunction::cast(*object)->should_have_prototype()) { Another place we could do Handle<JSFunction>::cast(object)->should_have_prototype() http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1203 src/ic.cc:1203: LookupForRead(object, name, &lookup); We should check if the raw version of LookupForRead is still used. http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1245 src/ic.cc:1245: Handle<Map> elements_map(receiver->elements()->map()); In the call code, we had handlified the non_strict_arguments_elements_map instead. It occurs to me that there is no great reason to have a handle for either, because we just immediately dereference it and it has only that one use. That is: if (receiver->elements()->map() == isolate()->heap()->non_strict_arguments_elements_map()) { ... http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1264 src/ic.cc:1264: } The function wants two blank lines after it. http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1266 src/ic.cc:1266: void KeyedLoadIC::UpdateCaches(LookupResult* lookup, State state, V8 style is one parameter per line if they don't all fit on the same line for declarations and definitions. http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1301 src/ic.cc:1301: Handle<AccessorInfo> callback(AccessorInfo::cast(*callback_object)); This is better: Handle<AccessorInfo> callback = Handle<AccessorInfo>::cast(callback_object); http://codereview.chromium.org/8352003/diff/1/src/ic.h File src/ic.h (right): http://codereview.chromium.org/8352003/diff/1/src/ic.h#newcode359 src/ic.h:359: return Handle<Code>(); I prefer the static factory function rather than the constructor: return Handle<Code>::null(); It's a little more verbose, but it's easier to spot due to the word 'null' and doesn't require understanding what the default constructor does. http://codereview.chromium.org/8352003/diff/1/src/stub-cache.cc File src/stub-cache.cc (right): http://codereview.chromium.org/8352003/diff/1/src/stub-cache.cc#newcode360 src/stub-cache.cc:360: compiler.CompileLoadConstant(name, receiver, holder, value); I guess this line is indented too far to the right. http://codereview.chromium.org/8352003/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
