http://codereview.chromium.org/993002/diff/1/7 File src/arm/assembler-arm.cc (right):
http://codereview.chromium.org/993002/diff/1/7#newcode1330 src/arm/assembler-arm.cc:1330: ASSERT(offset % 4 == 0); On 2010/03/16 11:04:56, Erik Corry wrote:
Here and in the function above we should assert that the offset is not
out of
range.
Done. http://codereview.chromium.org/993002/diff/1/7#newcode1407 src/arm/assembler-arm.cc:1407: On 2010/03/16 11:04:56, Erik Corry wrote:
There should be 2 blank lines here.
Some style issues like this were already fixed in patch set #3, but it seems you already started review when I uploaded it. http://codereview.chromium.org/993002/diff/1/7#newcode1411 src/arm/assembler-arm.cc:1411: On 2010/03/16 11:04:56, Erik Corry wrote:
And here.
Done. http://codereview.chromium.org/993002/diff/1/7#newcode1412 src/arm/assembler-arm.cc:1412: static bool is_signed(VFPType type) { On 2010/03/16 11:04:56, Erik Corry wrote:
Function names are CapitalCamelCase. This goes for all of them.
Done. http://codereview.chromium.org/993002/diff/1/7#newcode1423 src/arm/assembler-arm.cc:1423: On 2010/03/16 11:04:56, Erik Corry wrote:
Two lines here.
Done. http://codereview.chromium.org/993002/diff/1/7#newcode1454 src/arm/assembler-arm.cc:1454: int *Vm, On 2010/03/16 11:04:56, Erik Corry wrote:
Wrong placement of * and wrong capitalization of parameter name.
Done. http://codereview.chromium.org/993002/diff/1/7#newcode1468 src/arm/assembler-arm.cc:1468: const int dst_code, On 2010/03/16 11:04:56, Erik Corry wrote:
Wrong indentation.
Done. http://codereview.chromium.org/993002/diff/1/6 File src/arm/constants-arm.cc (right): http://codereview.chromium.org/993002/diff/1/6#newcode88 src/arm/constants-arm.cc:88: On 2010/03/16 11:04:56, Erik Corry wrote:
2 blank lines please.
Done. http://codereview.chromium.org/993002/diff/1/6#newcode88 src/arm/constants-arm.cc:88: On 2010/03/16 11:04:56, Erik Corry wrote:
2 blank lines please.
Done. http://codereview.chromium.org/993002/diff/1/6#newcode89 src/arm/constants-arm.cc:89: int VFPRegisters::Number(const char* name, bool *is_double) { On 2010/03/16 11:04:56, Erik Corry wrote:
Asterisk placement.
Done. http://codereview.chromium.org/993002/diff/1/6#newcode89 src/arm/constants-arm.cc:89: int VFPRegisters::Number(const char* name, bool *is_double) { On 2010/03/16 11:04:56, Erik Corry wrote:
Asterisk placement.
Done. http://codereview.chromium.org/993002/diff/1/10 File src/arm/disasm-arm.cc (right): http://codereview.chromium.org/993002/diff/1/10#newcode934 src/arm/disasm-arm.cc:934: if (instr->Bit(23) == 1) { On 2010/03/16 11:04:56, Erik Corry wrote:
This is getting very messy (not your fault) and I would like it
cleaned up. See
Table A7-24.
Done. http://codereview.chromium.org/993002/diff/1/11 File src/arm/ic-arm.cc (right): http://codereview.chromium.org/993002/diff/1/11#newcode644 src/arm/ic-arm.cc:644: // This functions would not work correctly for 0. On 2010/03/16 11:04:56, Erik Corry wrote:
functions would -> function does
Done. http://codereview.chromium.org/993002/diff/1/11#newcode650 src/arm/ic-arm.cc:650: const int meaningfull_bits On 2010/03/16 11:04:56, Erik Corry wrote:
meaningfull -> meaningful Please don't split over two lines if it fits on one.
Done. http://codereview.chromium.org/993002/diff/1/11#newcode654 src/arm/ic-arm.cc:654: = meaningfull_bits - HeapNumber::kMantissaBitsInTopWord; On 2010/03/16 11:04:56, Erik Corry wrote:
= should go on the line before (also 2 other places).
Done. http://codereview.chromium.org/993002/diff/1/11#newcode722 src/arm/ic-arm.cc:722: __ mov(r0, Operand(r0, ASR, kSmiTagSize)); // Untag key. On 2010/03/16 11:04:56, Erik Corry wrote:
You don't need to untag this. Instead you should assert that the
kSmiTag is 0
and that the kSmiTagSize is 1, then you should just leave r0 tagged.
Done. http://codereview.chromium.org/993002/diff/1/11#newcode789 src/arm/ic-arm.cc:789: __ vcvt_f64_s32(d0, s0); On 2010/03/16 11:04:56, Erik Corry wrote:
We can do vstr here.
Done. http://codereview.chromium.org/993002/diff/1/11#newcode866 src/arm/ic-arm.cc:866: // VFP is not available do manual single to double conversion. On 2010/03/16 11:04:56, Erik Corry wrote:
available do -> available, do
Done. http://codereview.chromium.org/993002/diff/1/11#newcode1058 src/arm/ic-arm.cc:1058: __ BranchOnNotSmi(r0, &slow); I tried this, but it does not pass tests. It seems that rounding mode for pixel arrays is round towards nearest, not round towards zero. On 2010/03/16 11:04:56, Erik Corry wrote:
You might use GetInt32 to do this without going into the runtime
system (a
different change probably).
http://codereview.chromium.org/993002 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
