Comments addressed, landing.
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())) {
On 2011/10/20 08:58:17, Kevin Millikin wrote:
I slightly prefer Handle<HeapNumber>::cast(key)->value() in handle
code.
Done.
http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1153
src/ic.cc:1153: Handle<Code> code =
On 2011/10/20 08:58:17, Kevin Millikin wrote:
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));
I prefer it too, but in this case
Handle<Code> code =
isolate()->stub_cache()->ComputeKeyedLoadStringLength(
is longer than 80 chars.
http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1169
src/ic.cc:1169: set_target(*code);
On 2011/10/20 08:58:17, Kevin Millikin wrote:
On 2011/10/19 13:47:47, ulan wrote:
> Add CHECK(!code.is_null()) here and below.
OK but ASSERT, not CHECK.
Done.
http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1173
src/ic.cc:1173: return Handle<JSArray>::cast(object)->length();
On 2011/10/20 08:58:17, Kevin Millikin wrote:
Is this just
return array->length()?
Done.
http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1179
src/ic.cc:1179: JSFunction::cast(*object)->should_have_prototype()) {
On 2011/10/20 08:58:17, Kevin Millikin wrote:
Another place we could do
Handle<JSFunction>::cast(object)->should_have_prototype()
Done.
http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1203
src/ic.cc:1203: LookupForRead(object, name, &lookup);
On 2011/10/20 08:58:17, Kevin Millikin wrote:
We should check if the raw version of LookupForRead is still used.
Done.
http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1245
src/ic.cc:1245: Handle<Map> elements_map(receiver->elements()->map());
On 2011/10/20 08:58:17, Kevin Millikin wrote:
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()) { ...
Done.
http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1264
src/ic.cc:1264: }
On 2011/10/20 08:58:17, Kevin Millikin wrote:
The function wants two blank lines after it.
Done.
http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1266
src/ic.cc:1266: void KeyedLoadIC::UpdateCaches(LookupResult* lookup,
State state,
On 2011/10/20 08:58:17, Kevin Millikin wrote:
V8 style is one parameter per line if they don't all fit on the same
line for
declarations and definitions.
Done.
http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1301
src/ic.cc:1301: Handle<AccessorInfo>
callback(AccessorInfo::cast(*callback_object));
On 2011/10/20 08:58:17, Kevin Millikin wrote:
This is better:
Handle<AccessorInfo> callback =
Handle<AccessorInfo>::cast(callback_object);
Done.
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>();
On 2011/10/20 08:58:17, Kevin Millikin wrote:
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.
Done.
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);
On 2011/10/20 08:58:17, Kevin Millikin wrote:
I guess this line is indented too far to the right.
Done.
http://codereview.chromium.org/8352003/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
