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
On 2010/02/04 15:05:06, Mads Ager wrote:
Should do a comment period sweep on this file as well.
Done.
http://codereview.chromium.org/561072/diff/1/40#newcode310
src/mips/assembler-mips.cc:310: return opcode == BEQ
On 2010/02/04 15:05:06, Mads Ager wrote:
Identation should be fixed for this return statement.
Done.
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;
On 2010/02/04 15:05:06, Mads Ager wrote:
No C-style casts please.
Done.
http://codereview.chromium.org/561072/diff/1/40#newcode464
src/mips/assembler-mips.cc:464: Instr instr = opcode | (rs.code() <<
kRsShift) | (rt.code() << kRtShift)
On 2010/02/04 15:05:06, Mads Ager wrote:
This should be indented according to Google style. Same goes for the
next
methods.
Done.
http://codereview.chromium.org/561072/diff/1/40#newcode842
src/mips/assembler-mips.cc:842: Instr instr = SPECIAL | TLTU | rs.code()
<< kRsShift |
On 2010/02/04 15:05:06, Mads Ager wrote:
Move the '|' to the next line for consistency with the rest of the
code.
Done.
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 |
On 2010/02/04 15:05:06, Mads Ager wrote:
Move the '|' to the next line for consistency with the rest of the
code.
Done.
http://codereview.chromium.org/561072/diff/1/40#newcode1171
src/mips/assembler-mips.cc:1171: CHECK(((instr1 & kOpcodeMask) == LUI &&
(instr2 & kOpcodeMask) == ORI) ||
On 2010/02/04 15:05:06, Mads Ager wrote:
Let's remove the extra white-spacing in the formatting in the rest of
this file?
Done.
http://codereview.chromium.org/561072/diff/1/40#newcode1205
src/mips/assembler-mips.cc:1205:
Removed #undef of macros no longer defined.
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; }
On 2010/02/04 15:05:06, Mads Ager wrote:
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.
Done.
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)
On 2010/02/04 15:05:06, Mads Ager wrote:
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?
Done.
http://codereview.chromium.org/561072/diff/1/47#newcode132
src/mips/assembler-mips.h:132: // Coprocessor register
On 2010/02/04 15:05:06, Mads Ager wrote:
End with a period.
Done.
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)
On 2010/02/04 15:05:06, Mads Ager wrote:
Comment style.
Done.
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
On 2010/02/04 15:05:06, Mads Ager wrote:
End comment with period.
Done.
http://codereview.chromium.org/561072/diff/1/47#newcode295
src/mips/assembler-mips.h:295: // Label operations & relative jumps
(PPUM Appendix D)
On 2010/02/04 15:05:06, Mads Ager wrote:
End with period.
Done.
http://codereview.chromium.org/561072/diff/1/47#newcode353
src/mips/assembler-mips.h:353: // target and the return address
On 2010/02/04 15:05:06, Mads Ager wrote:
End with period.
Done.
http://codereview.chromium.org/561072/diff/1/47#newcode368
src/mips/assembler-mips.h:368: // We don't use likely variant of
instructions
On 2010/02/04 15:05:06, Mads Ager wrote:
End with period.
Done.
http://codereview.chromium.org/561072/diff/1/47#newcode401
src/mips/assembler-mips.h:401: // Arithmetic
On 2010/02/04 15:05:06, Mads Ager wrote:
We should run through this file quickly and add periods at the end of
comments.
Done.
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
On 2010/02/04 15:05:06, Mads Ager wrote:
Uless -> below?
Done.
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.
On 2010/02/04 15:05:06, Mads Ager wrote:
All of the new files should just say "2010".
Done.
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 .
On 2010/02/04 15:05:06, Mads Ager wrote:
Remove spaces before period at the end.
Done.
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.
On 2010/02/04 15:05:06, Mads Ager wrote:
Remove, unneeded comment (people will do that anyway). :)
Done.
http://codereview.chromium.org/561072/diff/1/44#newcode54
src/mips/constants-mips.h:54: // Number of general purpose registers
On 2010/02/04 15:05:06, Mads Ager wrote:
Periods at end of comments.
Done.
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)
On 2010/02/04 15:05:06, Mads Ager wrote:
Add braces around the body.
Done.
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());
On 2010/02/04 15:05:06, Mads Ager wrote:
Align with the other arguments to fprintf.
Done.
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
On 2010/02/04 15:05:06, Mads Ager wrote:
Periods at end of comments.
Done.
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
On 2010/02/04 15:05:06, Mads Ager wrote:
Remove comment.
Done.
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);
On 2010/02/04 15:05:06, Mads Ager wrote:
Space around binary operator.
Done.
http://codereview.chromium.org/561072/diff/1/37#newcode375
src/mips/macro-assembler-mips.cc:375: for (int16_t i = 0; i<
kNumRegisters; i++) {
On 2010/02/04 15:05:06, Mads Ager wrote:
Space between 'i' and '<'.
Done.
http://codereview.chromium.org/561072/diff/1/37#newcode378
src/mips/macro-assembler-mips.cc:378: MemOperand(sp, 4*(NumToPush -
++NumSaved)));
On 2010/02/04 15:05:06, Mads Ager wrote:
Seems this will fit on the line above.
Space around binary operator.
Done.
http://codereview.chromium.org/561072/diff/1/37#newcode384
src/mips/macro-assembler-mips.cc:384: void
MacroAssembler::MultiPushReversed(RegList regs) {
On 2010/02/04 15:05:06, Mads Ager wrote:
Same comments as above.
Done.
http://codereview.chromium.org/561072/diff/1/37#newcode389
src/mips/macro-assembler-mips.cc:389: for (int16_t i = kNumRegisters;
--i >= 0;) {
On 2010/02/04 15:05:06, Mads Ager wrote:
for (int16_t i = kNumRegisters; i > 0; i--) {
Done.
http://codereview.chromium.org/561072/diff/1/37#newcode401
src/mips/macro-assembler-mips.cc:401: for (int16_t i = kNumRegisters;
--i >= 0;) {
On 2010/02/04 15:05:06, Mads Ager wrote:
for (int16_t i = kNumRegisters; i > 0; i--) {
Done.
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:
On 2010/02/04 15:05:06, Mads Ager wrote:
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.
Done.
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
On 2010/02/04 15:05:06, Mads Ager wrote:
Move comment to the line before the field.
Done.
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.
On 2010/02/04 15:05:06, Mads Ager wrote:
2010
Done.
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.",
On 2010/02/04 15:05:06, Mads Ager wrote:
Indentation.
Done.
http://codereview.chromium.org/561072/diff/1/50#newcode256
src/mips/simulator-mips.h:256: // Exceptions
On 2010/02/04 15:05:06, Mads Ager wrote:
Period.
Done.
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
On 2010/02/04 15:05:06, Mads Ager wrote:
Period at end of comment. What is included for macro-assembler
support?
Simplified include file list.
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
On 2010/02/04 15:05:06, Mads Ager wrote:
Periods at the end of comments.
Done.
http://codereview.chromium.org/561072
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
