Some initial comments.
http://codereview.chromium.org/549079/diff/1/5 File src/allocation.cc (right): http://codereview.chromium.org/549079/diff/1/5#newcode109 src/allocation.cc:109: void PreallocatedStorage::Init(size_t size) { Please revert this. http://codereview.chromium.org/549079/diff/1/22 File src/assembler.cc (right): http://codereview.chromium.org/549079/diff/1/22#newcode55 src/assembler.cc:55: #elif V8_TARGET_ARCH_MIPS I think we can safely ignore the native code RegExp implementation for now. Use regexp=interpreted when building or change the SConstruct file to default regexp to interpreted when building for mips. http://codereview.chromium.org/549079/diff/1/22#newcode648 src/assembler.cc:648: #elif V8_TARGET_ARCH_MIPS See above. http://codereview.chromium.org/549079/diff/1/17 File src/assembler.h (right): http://codereview.chromium.org/549079/diff/1/17#newcode512 src/assembler.h:512: static inline uint16_t NumBitsSet(uint32_t x) { Rename NumBitsSet to NumberOfBitsSet. How about just returning int? http://codereview.chromium.org/549079/diff/1/17#newcode514 src/assembler.h:514: for(int i=0; i<32; i++) { Please add spaces like this: for (int i = 0; i < 32; i++) { See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Horizontal_Whitespace#Horizontal_Whitespace. I don't know whether the performance of this function is important, but take a look at http://www-graphics.stanford.edu/~seander/bithacks.html#CountBitsSetNaive for alternatives. http://codereview.chromium.org/549079/diff/1/17#newcode515 src/assembler.h:515: if(x & 1<<i) num_bits_set++; Add spaces here like this if (x & (1 << i)) num_bits_set++; http://codereview.chromium.org/549079/diff/1/64 File src/checks.h (left): http://codereview.chromium.org/549079/diff/1/64#oldcode54 src/checks.h:54: Please revert this. http://codereview.chromium.org/549079/diff/1/11 File src/codegen.h (right): http://codereview.chromium.org/549079/diff/1/11#newcode153 src/codegen.h:153: #ifndef V8_TARGET_ARCH_MIPS Having a platform specific #ifdef in a platform independent file should be avoided. http://codereview.chromium.org/549079/diff/1/11#newcode156 src/codegen.h:156: inline void Branch(Condition cc, Register src1 = zero_reg, Look at http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Function_Declarations_and_Definitions#Function_Declarations_and_Definitions forformatting of parameter list. http://codereview.chromium.org/549079/diff/1/12 File src/compiler.cc (right): http://codereview.chromium.org/549079/diff/1/12#newcode123 src/compiler.cc:123: #ifndef V8_TARGET_ARCH_MIPS Please get rid of this #ifdef. You can create a FastCodeGenerator for MIPS which does bailout in all cases. http://codereview.chromium.org/549079/diff/1/26 File src/disasm.h (right): http://codereview.chromium.org/549079/diff/1/26#newcode42 src/disasm.h:42: #ifdef V8_TARGET_ARCH_MIPS This #ifdef should not be needed. Platform specific stuff has already spilled into this file (NameOfXMMRegister). That is not good practice, but not your fault. http://codereview.chromium.org/549079/diff/1/8 File src/flag-definitions.h (right): http://codereview.chromium.org/549079/diff/1/8#newcode208 src/flag-definitions.h:208: DEFINE_bool(trace_sim, true, "trace simulator execution") Please keep the default to false. http://codereview.chromium.org/549079/diff/1/66 File src/globals.h (right): http://codereview.chromium.org/549079/diff/1/66#newcode60 src/globals.h:60: #define V8_TARGET_CAN_READ_UNALIGNED 0 defining V8_TARGET_CAN_READ_UNALIGNED to 0 should not be needed. http://codereview.chromium.org/549079/diff/1/66#newcode180 src/globals.h:180: // On mips 0xbaddead is the encoding of jump to 0xeb77ab4. I don't think this is important. The handle zap value is placed where object pointers used to be, and will most likely be used as an object value and not executed code. Getting a segfault should be fine. http://codereview.chromium.org/549079/diff/1/4 File src/heap.cc (right): http://codereview.chromium.org/549079/diff/1/4#newcode47 src/heap.cc:47: #elif V8_TARGET_ARCH_MIPS && V8_NATIVE_REGEXP I think you can safely ignore native RegExp for MIPS for now. http://codereview.chromium.org/549079/diff/1/4#newcode1338 src/heap.cc:1338: Accidental edit? http://codereview.chromium.org/549079/diff/1/63 File src/jsregexp.cc (right): http://codereview.chromium.org/549079/diff/1/63#newcode53 src/jsregexp.cc:53: #elif V8_TARGET_ARCH_MIPS I think you can safely ignore native RegExp for MIPS for now. http://codereview.chromium.org/549079/diff/1/63#newcode4481 src/jsregexp.cc:4481: #elif V8_TARGET_ARCH_MIPS I think you can safely ignore native RegExp for MIPS for now. http://codereview.chromium.org/549079/diff/1/23 File src/jump-target.h (right): http://codereview.chromium.org/549079/diff/1/23#newcode116 src/jump-target.h:116: #ifndef V8_TARGET_ARCH_MIPS Like in codegen.h having a platform specific #ifdef in a platform independent file should be avoided. http://codereview.chromium.org/549079/diff/1/23#newcode120 src/jump-target.h:120: virtual void Branch(Condition cc, Hint hint = no_hint, Register src1 = zero_reg, const Operand& src2 = Operand(zero_reg)); Long line, see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Line_Length#Line_Length. http://codereview.chromium.org/549079/diff/1/23#newcode161 src/jump-target.h:161: #ifndef V8_TARGET_ARCH_MIPS See above. http://codereview.chromium.org/549079/diff/1/23#newcode164 src/jump-target.h:164: void DoBranch(Condition cc, Hint hint, Register src1 = zero_reg, const Operand& src2 = Operand(zero_reg)); See above. http://codereview.chromium.org/549079/diff/1/23#newcode221 src/jump-target.h:221: // code after the branch. See above. http://codereview.chromium.org/549079/diff/1/51 File src/mips/assembler-mips.cc (right): http://codereview.chromium.org/549079/diff/1/51#newcode1 src/mips/assembler-mips.cc:1: // Copyright 2006-2008 the V8 project authors. All rights reserved. Please use the copyright header from src/mips/assembler-mips-inl.h mentioning Sun Microsystems Inc. here as well. http://codereview.chromium.org/549079/diff/1/58 File src/mips/assembler-mips.h (right): http://codereview.chromium.org/549079/diff/1/58#newcode1 src/mips/assembler-mips.h:1: // Copyright 2006-2008 the V8 project authors. All rights reserved. Please use the copyright header from src/mips/assembler-mips-inl.h mentioning Sun Microsystems Inc. here as well. http://codereview.chromium.org/549079/diff/1/30 File src/mips/cfg-mips.cc (right): http://codereview.chromium.org/549079/diff/1/30#newcode1 src/mips/cfg-mips.cc:1: // You can safely remove this file as using a control flow graph was an experiment which has been abandonned. http://codereview.chromium.org/549079/diff/1/34 File src/mips/codegen-mips.cc (right): http://codereview.chromium.org/549079/diff/1/34#newcode1 src/mips/codegen-mips.cc:1: // Copyright 2006-2008 the V8 project authors. All rights reserved. Please remove all the code in comments. http://codereview.chromium.org/549079/diff/1/49 File src/mips/codegen-mips.h (right): http://codereview.chromium.org/549079/diff/1/49#newcode466 src/mips/codegen-mips.h:466: // // Encode the parameters in a unique 16 bit value. Please remove code in comments. http://codereview.chromium.org/549079/diff/1/49#newcode477 src/mips/codegen-mips.h:477: // if (constant_rhs == CodeGenerator::kUnknownIntValue) return false; Ditto. http://codereview.chromium.org/549079/diff/1/49#newcode490 src/mips/codegen-mips.h:490: // if (!specialized_on_rhs_) return 0; Ditto. http://codereview.chromium.org/549079/diff/1/49#newcode522 src/mips/codegen-mips.h:522: // if (!specialized_on_rhs_) { Ditto. http://codereview.chromium.org/549079/diff/1/54 File src/mips/constants-mips.h (right): http://codereview.chromium.org/549079/diff/1/54#newcode13 src/mips/constants-mips.h:13: // ( http://web.asic.sdesigns.com/tools/libs/mips/doc/74K/doc/MIPS32-ISA.pdf ) This link seems not to work. How about using a www.mips.com URL? Even if it requires login. http://codereview.chromium.org/549079/diff/1/54#newcode112 src/mips/constants-mips.h:112: J = ( (0<<3) + 2 ) << opcode_o, Formatting like ((0 << 3) + 2) << opcode_o matches the rest of the V8 code better. http://codereview.chromium.org/549079/diff/1/54#newcode128 src/mips/constants-mips.h:128: COP1 = ( (2<<3) + 1 ) << opcode_o, // Coprocessor 1 class (floating point) Long line. http://codereview.chromium.org/549079/diff/1/54#newcode350 src/mips/constants-mips.h:350: return Bits(rs_o+rs_l-1, rs_o); Format expression with spaces rs_o + rs_l - 1. http://codereview.chromium.org/549079/diff/1/19 File src/platform-linux.cc (right): http://codereview.chromium.org/549079/diff/1/19#newcode98 src/platform-linux.cc:98: #ifdef V8_TARGET_ARCH_MIPS Combine with #else above to #elif. http://codereview.chromium.org/549079
-- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
