LGTM
This is mostly a rubberstamp. I did have a quick look through it and have
added
a number of nits. Overall, it looks very nice.
http://codereview.chromium.org/561072/diff/1/40
File src/mips/assembler-mips.cc (right):
http://codereview.chromium.org/561072/diff/1/40#newcode177
src/mips/assembler-mips.cc:177: // Implementation of RelocInfo
Should do a comment period sweep on this file as well.
http://codereview.chromium.org/561072/diff/1/40#newcode310
src/mips/assembler-mips.cc:310: return opcode == BEQ
Identation should be fixed for this return statement.
http://codereview.chromium.org/561072/diff/1/40#newcode336
src/mips/assembler-mips.cc:336: int32_t imm18 = (((int32_t)instr &
(int32_t)kImm16Mask) << 16) >> 14;
No C-style casts please.
http://codereview.chromium.org/561072/diff/1/40#newcode464
src/mips/assembler-mips.cc:464: Instr instr = opcode | (rs.code() <<
kRsShift) | (rt.code() << kRtShift)
This should be indented according to Google style. Same goes for the
next methods.
http://codereview.chromium.org/561072/diff/1/40#newcode842
src/mips/assembler-mips.cc:842: Instr instr = SPECIAL | TLTU | rs.code()
<< kRsShift |
Move the '|' to the next line for consistency with the rest of the code.
http://codereview.chromium.org/561072/diff/1/40#newcode997
src/mips/assembler-mips.cc:997: Instr instr = COP1 | fmt | ft.code() <<
16 | fs.code() << kFsShift |
Move the '|' to the next line for consistency with the rest of the code.
http://codereview.chromium.org/561072/diff/1/40#newcode1171
src/mips/assembler-mips.cc:1171: CHECK(((instr1 & kOpcodeMask) == LUI &&
(instr2 & kOpcodeMask) == ORI) ||
Let's remove the extra white-spacing in the formatting in the rest of
this file?
http://codereview.chromium.org/561072/diff/1/47
File src/mips/assembler-mips.h (right):
http://codereview.chromium.org/561072/diff/1/47#newcode79
src/mips/assembler-mips.h:79: bool is_byte_register() const { return
false; }
If there is no such distinction, we should just remove this method and
the comment. This method is not there in the ARM nor the x64 version
either.
http://codereview.chromium.org/561072/diff/1/47#newcode89
src/mips/assembler-mips.h:89: // (unfortunately we can't make this
private in a struct)
This is just copied from the other assemblers but could we change this
comment in all the assemblers to remove the parenthesis and follow the
convention of capitalizing and ending with a period?
http://codereview.chromium.org/561072/diff/1/47#newcode132
src/mips/assembler-mips.h:132: // Coprocessor register
End with a period.
http://codereview.chromium.org/561072/diff/1/47#newcode145
src/mips/assembler-mips.h:145: // (unfortunately we can't make this
private in a struct)
Comment style.
http://codereview.chromium.org/561072/diff/1/47#newcode227
src/mips/assembler-mips.h:227: // Class Operand represents a shifter
operand in data processing instructions
End comment with period.
http://codereview.chromium.org/561072/diff/1/47#newcode295
src/mips/assembler-mips.h:295: // Label operations & relative jumps
(PPUM Appendix D)
End with period.
http://codereview.chromium.org/561072/diff/1/47#newcode353
src/mips/assembler-mips.h:353: // target and the return address
End with period.
http://codereview.chromium.org/561072/diff/1/47#newcode368
src/mips/assembler-mips.h:368: // We don't use likely variant of
instructions
End with period.
http://codereview.chromium.org/561072/diff/1/47#newcode401
src/mips/assembler-mips.h:401: // Arithmetic
We should run through this file quickly and add periods at the end of
comments.
http://codereview.chromium.org/561072/diff/1/47#newcode578
src/mips/assembler-mips.h:578: // The relocation writer's position is at
least kGap bytes Uless the end of
Uless -> below?
http://codereview.chromium.org/561072/diff/1/27
File src/mips/builtins-mips.cc (right):
http://codereview.chromium.org/561072/diff/1/27#newcode1
src/mips/builtins-mips.cc:1: // Copyright 2006-2010 the V8 project
authors. All rights reserved.
All of the new files should just say "2010".
http://codereview.chromium.org/561072/diff/1/44
File src/mips/constants-mips.h (right):
http://codereview.chromium.org/561072/diff/1/44#newcode45
src/mips/constants-mips.h:45: // Try
www.cs.cornell.edu/courses/cs3410/2008fa/MIPS_Vol2.pdf .
Remove spaces before period at the end.
http://codereview.chromium.org/561072/diff/1/44#newcode46
src/mips/constants-mips.h:46: // Else e quick Google search and you'll
find it.
Remove, unneeded comment (people will do that anyway). :)
http://codereview.chromium.org/561072/diff/1/44#newcode54
src/mips/constants-mips.h:54: // Number of general purpose registers
Periods at end of comments.
http://codereview.chromium.org/561072/diff/1/26
File src/mips/cpu-mips.cc (right):
http://codereview.chromium.org/561072/diff/1/26#newcode54
src/mips/cpu-mips.cc:54: if (res)
Add braces around the body.
http://codereview.chromium.org/561072/diff/1/48
File src/mips/disasm-mips.cc (right):
http://codereview.chromium.org/561072/diff/1/48#newcode777
src/mips/disasm-mips.cc:777: prev_pc,
*reinterpret_cast<int32_t*>(prev_pc), buffer.start());
Align with the other arguments to fprintf.
http://codereview.chromium.org/561072/diff/1/45
File src/mips/frames-mips.h (right):
http://codereview.chromium.org/561072/diff/1/45#newcode124
src/mips/frames-mips.h:124: // C/C++ argument slots size
Periods at end of comments.
http://codereview.chromium.org/561072/diff/1/31
File src/mips/jump-target-mips.cc (right):
http://codereview.chromium.org/561072/diff/1/31#newcode29
src/mips/jump-target-mips.cc:29: // FROM ARM Code
Remove comment.
http://codereview.chromium.org/561072/diff/1/37
File src/mips/macro-assembler-mips.cc (right):
http://codereview.chromium.org/561072/diff/1/37#newcode374
src/mips/macro-assembler-mips.cc:374: addiu(sp, sp, -4*NumToPush);
Space around binary operator.
http://codereview.chromium.org/561072/diff/1/37#newcode375
src/mips/macro-assembler-mips.cc:375: for (int16_t i = 0; i<
kNumRegisters; i++) {
Space between 'i' and '<'.
http://codereview.chromium.org/561072/diff/1/37#newcode378
src/mips/macro-assembler-mips.cc:378: MemOperand(sp, 4*(NumToPush -
++NumSaved)));
Seems this will fit on the line above.
Space around binary operator.
http://codereview.chromium.org/561072/diff/1/37#newcode384
src/mips/macro-assembler-mips.cc:384: void
MacroAssembler::MultiPushReversed(RegList regs) {
Same comments as above.
http://codereview.chromium.org/561072/diff/1/37#newcode389
src/mips/macro-assembler-mips.cc:389: for (int16_t i = kNumRegisters;
--i >= 0;) {
for (int16_t i = kNumRegisters; i > 0; i--) {
http://codereview.chromium.org/561072/diff/1/37#newcode401
src/mips/macro-assembler-mips.cc:401: for (int16_t i = kNumRegisters;
--i >= 0;) {
for (int16_t i = kNumRegisters; i > 0; i--) {
http://codereview.chromium.org/561072/diff/1/41
File src/mips/macro-assembler-mips.h (right):
http://codereview.chromium.org/561072/diff/1/41#newcode61
src/mips/macro-assembler-mips.h:61: private:
Strange mix of public and private here. I would prefer to have all the
public stuff at the beginning and move all the private stuff to the end.
We should do that for the other macro assemblers too if they have the
same structure.
http://codereview.chromium.org/561072/diff/1/41#newcode354
src/mips/macro-assembler-mips.h:354: Handle<Object> code_object_; //
This handle will be patched with the code
Move comment to the line before the field.
http://codereview.chromium.org/561072/diff/1/51
File src/mips/register-allocator-mips-inl.h (right):
http://codereview.chromium.org/561072/diff/1/51#newcode1
src/mips/register-allocator-mips-inl.h:1: // Copyright 2006-2008 the V8
project authors. All rights reserved.
2010
http://codereview.chromium.org/561072/diff/1/50
File src/mips/simulator-mips.h (right):
http://codereview.chromium.org/561072/diff/1/50#newcode241
src/mips/simulator-mips.h:241: "Eror:Unexpected %i opcode in a branch
delay slot.",
Indentation.
http://codereview.chromium.org/561072/diff/1/50#newcode256
src/mips/simulator-mips.h:256: // Exceptions
Period.
http://codereview.chromium.org/561072/diff/1/5
File test/cctest/test-assembler-mips.cc (right):
http://codereview.chromium.org/561072/diff/1/5#newcode34
test/cctest/test-assembler-mips.cc:34: // For macro-assembler support
Period at end of comment. What is included for macro-assembler support?
http://codereview.chromium.org/561072/diff/1/5#newcode54
test/cctest/test-assembler-mips.cc:54: // The test framework does not
accept flags on the command line, so we set them
Periods at the end of comments.
http://codereview.chromium.org/561072
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
