[v8-dev] Re: Forking disassembler and simulator for Thumb2 support; (issue651029)

[ager]

A couple of overall comments.  I'll let Erik review the actual  
functionality.

http://codereview.chromium.org/651029/diff/3020/4046
File src/SConscript (right):

http://codereview.chromium.org/651029/diff/3020/4046#newcode122
src/SConscript:122: arm/instr-thumb2.cc
Can we get rid of this new file please?  We should follow the code
organization from the other assemblers so that things are not different
for just one of our backends.

http://codereview.chromium.org/651029/diff/3020/4056
File src/arm/assembler-thumb2-inl.h (right):

http://codereview.chromium.org/651029/diff/3020/4056#newcode150
src/arm/assembler-thumb2-inl.h:150: return (Assembler::instr_arm_at(pc_)
== kMovLrPc) &&
Funky indentation here.  This is not your code, but I think we should
rewrite this overly complex return statement.  Let's do something like
(and change it in the other assembler as well):

if (Assembler::instr_arm_at(pc_) == kMovLrPc) {
  Instr instr = Assembler::instr_arm_at(pc_ + Assembler::kInstrArmSize);
  return (instr & kLdrPcPattern) == kLdrPcPattern;
}
return false;

http://codereview.chromium.org/651029/diff/3020/4052
File src/arm/assembler-thumb2.h (right):

http://codereview.chromium.org/651029/diff/3020/4052#newcode104
src/arm/assembler-thumb2.h:104: enum Bits1 {
I would like to get rid of these Bits enums.

http://codereview.chromium.org/651029/diff/3020/4053
File src/arm/constants-arm.h (right):

http://codereview.chromium.org/651029/diff/3020/4053#newcode132
src/arm/constants-arm.h:132: enum Bits1 {
Let's get rid of these.

http://codereview.chromium.org/651029/diff/3020/4057
File src/arm/instr-thumb2.h (right):

http://codereview.chromium.org/651029/diff/3020/4057#newcode45
src/arm/instr-thumb2.h:45: enum Operation {
Why do you need this enum?  There is nothing like this in the normal ARM
assembler and we should not diverge from that unless we really have to.

http://codereview.chromium.org/651029/diff/3020/4057#newcode135
src/arm/instr-thumb2.h:135: class InstrThumb2 {
Again, it seems to me that we should follow the style of the other
assemblers.  Having something different in the thumb2 assembler will
just be confusing when developing for all platforms.

I think we should get rid of these new files and follow the style of the
other assemblers.

http://codereview.chromium.org/651029

--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev