https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc
File src/hydrogen.cc (left):
https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#oldcode7284
src/hydrogen.cc:7284: HValue* constructor =
HPushArgument::cast(Top())->argument();
Why is it ok to merge this with the code above? The code above contains
the function on the TOS, this code contains an HPushArgument(function)
on the TOS.
https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7187
src/hydrogen.cc:7187: HValue* argument =
environment()->ExpressionStackAt(0);
Top();
https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7192
src/hydrogen.cc:7192: int value = constant_argument->Integer32Value();
Rename value to array_size.
https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7203
src/hydrogen.cc:7203: : array_builder.AllocateArray(argument, argument,
true);
Can we replace that "true" with an enum indicating the meaning?
https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7270
src/hydrogen.cc:7270: ASSERT(argument_count <= 5);
Why 5? Can we at least make it a descriptive constant somewhere?
https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7278
src/hydrogen.cc:7278: new_object = array_builder.AllocateArray(length,
length, fill_with_hole);
Why do we only fill with holes if the kind is smi? At least add a
comment.
https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7306
src/hydrogen.cc:7306: if (argument_count <= 5) {
Why 5?
https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7309
src/hydrogen.cc:7309: if (!site->DoNotInlineCall()) {
What about just CanInlineCall() rather than the double negative?
https://codereview.chromium.org/55933002/diff/170001/src/hydrogen.cc#newcode7325
src/hydrogen.cc:7325: // inline this case.
Shouldn't we just generate a check whether the argument is 0 and bail
out if not?
https://codereview.chromium.org/55933002/diff/170001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):
https://codereview.chromium.org/55933002/diff/170001/src/ia32/code-stubs-ia32.cc#newcode5767
src/ia32/code-stubs-ia32.cc:5767: Immediate(2));
I don't know what the line above means.
https://codereview.chromium.org/55933002/diff/170001/src/ia32/code-stubs-ia32.cc#newcode5768
src/ia32/code-stubs-ia32.cc:5768: /*
leftover comment?
https://codereview.chromium.org/55933002/diff/170001/src/objects-inl.h
File src/objects-inl.h (right):
https://codereview.chromium.org/55933002/diff/170001/src/objects-inl.h#newcode1373
src/objects-inl.h:1373: if (reason == TENURING) {
Can we make this a switch to statically ensure we catch all cases?
https://codereview.chromium.org/55933002/diff/170001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/55933002/diff/170001/src/objects.cc#newcode12752
src/objects.cc:12752: if (SitePointsToLiteral() &&
nit: merge lines
https://codereview.chromium.org/55933002/diff/170001/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/55933002/diff/170001/src/objects.h#newcode8086
src/objects.h:8086: static const int kElementsKindMask = 0xffff;
Is there a reason why you don't use BitFields?
https://codereview.chromium.org/55933002/diff/170001/src/runtime.cc
File src/runtime.cc (right):
https://codereview.chromium.org/55933002/diff/170001/src/runtime.cc#newcode14667
src/runtime.cc:14667: Handle<AllocationSite> site;
Can we just declare it below, right above the if where it's initialized?
https://codereview.chromium.org/55933002/diff/170001/src/runtime.cc#newcode14691
src/runtime.cc:14691: if (!site.is_null() && !can_use_type_feedback) {
I find all these !site.is_null() a bit hard to read. Can we reorder the
branches and/or extract code into separate functions?
https://codereview.chromium.org/55933002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
