This looks great!
A couple of indentation changes and suggestions below. Once they are
addressed I
think we are ready to land this.
We are very inconsistent in our enumerator names. I have put in a comment
on the
things that I think we should fix before landing this patch. The rest we can
look at in follow up patches.
http://codereview.chromium.org/6274009/diff/21001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):
http://codereview.chromium.org/6274009/diff/21001/src/arm/assembler-arm.cc#newcode432
src/arm/assembler-arm.cc:432: return ((instr & ~RdMask) ==
kPushRegPattern);
RdMask is a constant. We should keep the kRdMask name for it.
http://codereview.chromium.org/6274009/diff/21001/src/arm/assembler-arm.cc#newcode491
src/arm/assembler-arm.cc:491: if ((Instruction::ConditionField(instr) ==
special_condition) &&
kSpecialCondition?
http://codereview.chromium.org/6274009/diff/21001/src/arm/assembler-arm.cc#newcode1462
src/arm/assembler-arm.cc:1462: } else if
(Instruction::RdValue(mem_write_instr)
Not your change, but could you format this like the other cases please?
http://codereview.chromium.org/6274009/diff/21001/src/arm/assembler-arm.cc#newcode1470
src/arm/assembler-arm.cc:1470: Instruction::RdValue(ldr_instr)) ||
Align with Instruction::RdValue on the line above.
http://codereview.chromium.org/6274009/diff/21001/src/arm/assembler-arm.cc#newcode1599
src/arm/assembler-arm.cc:1599: svc(stop_code + code, cond);
Is stop_code a constant? In that case it should be named as one:
kStopCode.
http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h
File src/arm/constants-arm.h (right):
http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h#newcode119
src/arm/constants-arm.h:119: no_condition = -1,
kNoCondition
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumerator_Names#Enumerator_Names
http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h#newcode121
src/arm/constants-arm.h:121: eq = 0 << 28, // Z set
Equal.
These really should be capitalized, but in order to not have to change
everything I'm OK with leaving these.
http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h#newcode137
src/arm/constants-arm.h:137: special_condition = 15 << 28, // Special
condition (refer to section A3.2.1).
kSpecialCondition?
http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h#newcode138
src/arm/constants-arm.h:138: number_of_conditions = 16,
kNumberOfConditions?
http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h#newcode256
src/arm/constants-arm.h:256: // Instruction bit masks.
Prefix all of these with a 'k': kCondMask, kALUMask, ...
http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h#newcode296
src/arm/constants-arm.h:296: number_of_shifts = 4
kNumberOfShifts
http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h#newcode364
src/arm/constants-arm.h:364: call_rt_redirected = 0x10,
Please use kCamelCase naming for these.
http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc
File src/arm/disasm-arm.cc (right):
http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newcode235
src/arm/disasm-arm.cc:235: ", %s ", shift_names[shift_index]);
Identation.
http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newcode248
src/arm/disasm-arm.cc:248: "#%d", imm);
Indentation.
http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newcode257
src/arm/disasm-arm.cc:257: ", %s #%d",
Indentation.
http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newcode304
src/arm/disasm-arm.cc:304: "%d - 0x%x",
Indentation.
http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newcode309
src/arm/disasm-arm.cc:309: "%d",
Indentation.
http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newcode405
src/arm/disasm-arm.cc:405: ", #%d", imm);
Indentation. And more occurrences below.
http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newcode1358
src/arm/disasm-arm.cc:1358: namespace v8i = v8::internal;
I don't like the v8i shorthand. Remove and just use v8::internal as
before.
Alternatively, is there any reason to actually have the disassembler
support in a different namespace. Why not have that in the v8::internal
namespace as well?
http://codereview.chromium.org/6274009/diff/21001/src/arm/simulator-arm.h
File src/arm/simulator-arm.h (right):
http://codereview.chromium.org/6274009/diff/21001/src/arm/simulator-arm.h#newcode355
src/arm/simulator-arm.h:355: } } // namespace v8::internal
Remove lines 355-359? Leaving and re-entering the same namespace.
http://codereview.chromium.org/6274009/
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
