Just saw a last comment on a few words' case. I will fix it.
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); On 2011/01/24 11:45:03, Mads Ager wrote:
RdMask is a constant. We should keep the kRdMask name for it.
Done. 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) && On 2011/01/24 11:45:03, Mads Ager wrote:
kSpecialCondition?
Done. 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) On 2011/01/24 11:45:03, Mads Ager wrote:
Not your change, but could you format this like the other cases
please? Done. http://codereview.chromium.org/6274009/diff/21001/src/arm/assembler-arm.cc#newcode1470 src/arm/assembler-arm.cc:1470: Instruction::RdValue(ldr_instr)) || On 2011/01/24 11:45:03, Mads Ager wrote:
Align with Instruction::RdValue on the line above.
Wasn't exactly sure of the best indentation . I hope this works. http://codereview.chromium.org/6274009/diff/21001/src/arm/assembler-arm.cc#newcode1599 src/arm/assembler-arm.cc:1599: svc(stop_code + code, cond); It is. Renamed. On 2011/01/24 11:45:03, Mads Ager wrote:
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, On 2011/01/24 11:45:03, Mads Ager wrote:
kNoCondition
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumerator_Names#Enumerator_Names Done. 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. I leave it then. I think it'll also fit better with the rest of the code and other archs. On 2011/01/24 11:45:03, Mads Ager wrote:
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). Initially named all these to be coherent with lower-case conditions. I'll use kCamelCase for all non standard conditions. If we use kNoCondition I think On 2011/01/24 11:45:03, Mads Ager wrote:
kSpecialCondition?
http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h#newcode138 src/arm/constants-arm.h:138: number_of_conditions = 16, Same. Done. On 2011/01/24 11:45:03, Mads Ager wrote:
kNumberOfConditions?
http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h#newcode256 src/arm/constants-arm.h:256: // Instruction bit masks. On 2011/01/24 11:45:03, Mads Ager wrote:
Prefix all of these with a 'k': kCondMask, kALUMask, ...
Done. http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h#newcode296 src/arm/constants-arm.h:296: number_of_shifts = 4 On 2011/01/24 11:45:03, Mads Ager wrote:
kNumberOfShifts
Done. 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]); On 2011/01/24 11:45:03, Mads Ager wrote:
Identation.
Done. http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newcode248 src/arm/disasm-arm.cc:248: "#%d", imm); On 2011/01/24 11:45:03, Mads Ager wrote:
Indentation.
Done. http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newcode257 src/arm/disasm-arm.cc:257: ", %s #%d", On 2011/01/24 11:45:03, Mads Ager wrote:
Indentation.
Done. http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newcode304 src/arm/disasm-arm.cc:304: "%d - 0x%x", On 2011/01/24 11:45:03, Mads Ager wrote:
Indentation.
Done. http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newcode309 src/arm/disasm-arm.cc:309: "%d", On 2011/01/24 11:45:03, Mads Ager wrote:
Indentation.
Done. http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newcode405 src/arm/disasm-arm.cc:405: ", #%d", imm); On 2011/01/24 11:45:03, Mads Ager wrote:
Indentation. And more occurrences below.
Done. http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newcode1358 src/arm/disasm-arm.cc:1358: namespace v8i = v8::internal; Replaced v8i with v8::internal. About the disasm namespace, I can't just remove it on ARM. The disasm namespace is used for all architectures, and there are 2 Disassembler classes (disasm.h and disassembler.h) distinguished through this namespace. On 2011/01/24 11:45:03, Mads Ager wrote:
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 On 2011/01/24 11:45:03, Mads Ager wrote:
Remove lines 355-359? Leaving and re-entering the same namespace.
Done. http://codereview.chromium.org/6274009/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
