Just stylistic comments.
http://codereview.chromium.org/549079/diff/1/31 File src/mips/assembler-mips-inl.h (right): http://codereview.chromium.org/549079/diff/1/31#newcode116 src/mips/assembler-mips-inl.h:116: return Assembler::target_address_at(pc_); nit: wrong indent, -2 spaces. http://codereview.chromium.org/549079/diff/1/31#newcode140 src/mips/assembler-mips-inl.h:140: return Handle<Object>(reinterpret_cast<Object**>(Assembler::target_address_at(pc_))); not: line is too long, ./tools/presubmit.py should have let you known that http://codereview.chromium.org/549079/diff/1/31#newcode197 src/mips/assembler-mips-inl.h:197: printf("%s - %d - %s : Checking for jal or jalr. \ I think PrintF should be preferred. And probably in #ifdef DEBUG http://codereview.chromium.org/549079/diff/1/31#newcode203 src/mips/assembler-mips-inl.h:203: ); slightly strange indentation, I'd lift ); to the previous line. http://codereview.chromium.org/549079/diff/1/51 File src/mips/assembler-mips.cc (right): http://codereview.chromium.org/549079/diff/1/51#newcode312 src/mips/assembler-mips.cc:312: // Checks if the instruction is a branch or a jump (jr and jalr excluded) I think this line should be indented as well. http://codereview.chromium.org/549079/diff/1/51#newcode338 src/mips/assembler-mips.cc:338: int32_t imm18 = (((int32_t)instr & (int32_t)Imm16Mask) << 16) >>14; nit: a space between >> and 14 http://codereview.chromium.org/549079/diff/1/51#newcode589 src/mips/assembler-mips.cc:589: if(rt.is_reg()) { nit: a space between if and ( http://codereview.chromium.org/549079/diff/1/51#newcode654 src/mips/assembler-mips.cc:654: if(rt.is_reg()) { nit: space between if and ( http://codereview.chromium.org/549079/diff/1/51#newcode675 src/mips/assembler-mips.cc:675: // Signed comparison nit: -2 spaces http://codereview.chromium.org/549079/diff/1/51#newcode693 src/mips/assembler-mips.cc:693: // Unsigned comparison. ditto http://codereview.chromium.org/549079/diff/1/51#newcode745 src/mips/assembler-mips.cc:745: // Signed comparison ditto http://codereview.chromium.org/549079/diff/1/51#newcode767 src/mips/assembler-mips.cc:767: // Unsigned comparison. ditto http://codereview.chromium.org/549079/diff/1/51#newcode797 src/mips/assembler-mips.cc:797: if(rt.is_reg()) { if ( http://codereview.chromium.org/549079/diff/1/51#newcode819 src/mips/assembler-mips.cc:819: // Signed comparison ditto http://codereview.chromium.org/549079/diff/1/51#newcode841 src/mips/assembler-mips.cc:841: // Unsigned comparison. ditto http://codereview.chromium.org/549079/diff/1/51#newcode939 src/mips/assembler-mips.cc:939: if(!MustUse_at(rt.rmode_)) { if (. And below http://codereview.chromium.org/549079/diff/1/51#newcode940 src/mips/assembler-mips.cc:940: emit(J | rt.imm32_); +2 spaces http://codereview.chromium.org/549079/diff/1/51#newcode1320 src/mips/assembler-mips.cc:1320: // Load, store, move why additional indenting? http://codereview.chromium.org/549079/diff/1/51#newcode1699 src/mips/assembler-mips.cc:1699: // return Memory::Address_at(target_address_address_at(pc)); remove? http://codereview.chromium.org/549079/diff/1/51#newcode1722 src/mips/assembler-mips.cc:1722: // We should never get here. UNREACHABLE? http://codereview.chromium.org/549079/diff/1/51#newcode1724 src/mips/assembler-mips.cc:1724: remove empty line? http://codereview.chromium.org/549079/diff/1/58 File src/mips/assembler-mips.h (right): http://codereview.chromium.org/549079/diff/1/58#newcode69 src/mips/assembler-mips.h:69: return 0 <= code_ && code_ < 32; } nit: I think v8 style would be { ... } on one line or bool is_valid() const { ... } http://codereview.chromium.org/549079/diff/1/58#newcode252 src/mips/assembler-mips.h:252: // Class MemOperand represents a memory operand in load and store instructions Usually all comments are terminated with a dot (here and everywhere.) http://codereview.chromium.org/549079/diff/1/58#newcode331 src/mips/assembler-mips.h:331: // This sets the branch destination (which is in the instruction on x86). x86? http://codereview.chromium.org/549079/diff/1/58#newcode368 src/mips/assembler-mips.h:368: // void beql (Register rs, Register rt, int16_t offset); maybe those commented out lines should be just removed? http://codereview.chromium.org/549079/diff/1/28 File src/mips/codegen-mips-inl.h (right): http://codereview.chromium.org/549079/diff/1/28#newcode2 src/mips/codegen-mips-inl.h:2: // Redistribution and use in source and binary forms, with or without I don't know what others would say, but maybe you don't need those -inl.h files---this style gets deprecated. But consistency may trump (my) comfort in this case. http://codereview.chromium.org/549079/diff/1/36 File src/mips/constants-mips.cc (right): http://codereview.chromium.org/549079/diff/1/36#newcode133 src/mips/constants-mips.cc:133: return true; ditto http://codereview.chromium.org/549079/diff/1/54 File src/mips/constants-mips.h (right): http://codereview.chromium.org/549079/diff/1/54#newcode401 src/mips/constants-mips.h:401: break; why break after return? http://codereview.chromium.org/549079/diff/1/52 File src/mips/test-interface-mips.cc (right): http://codereview.chromium.org/549079/diff/1/52#newcode1 src/mips/test-interface-mips.cc:1: #include <stdint.h> shouldn't those be moved into test dir? http://codereview.chromium.org/549079
-- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
