On Thu, Feb 11, 2010 at 2:36 PM, <[email protected]> wrote: > I'm not done yet. Here's some feedback to be getting on with. > > > http://codereview.chromium.org/601028/diff/1/9 > File src/arm/assembler-thumb2-inl.h (right): > > http://codereview.chromium.org/601028/diff/1/9#newcode151 > src/arm/assembler-thumb2-inl.h:151: && ((Assembler::instrArm_at(pc_ + > Assembler::kInstrArmSize) & kLdrPCPattern) > This line is too long. >
Fixed > > You need to install cpplint.py and run tools/presubmit.py > > Fixed other lint issues > http://codereview.chromium.org/601028/diff/1/9#newcode223 > src/arm/assembler-thumb2-inl.h:223: void Assembler::CheckArmMode() { > These should be called EnsureXxx instead of CheckXxx to be consistent > with the naming elsewhere in the VM. > Changed. > > http://codereview.chromium.org/601028/diff/1/9#newcode226 > src/arm/assembler-thumb2-inl.h:226: if (pc_offset() & 2) { > Please do not use implicit conversions to bool (ie add != 0) > fixed > > http://codereview.chromium.org/601028/diff/1/9#newcode229 > src/arm/assembler-thumb2-inl.h:229: // emitThumb(16*B12| 8 * B5 | > pc.code()); > We don't allow commented out code in V8. > removed > > http://codereview.chromium.org/601028/diff/1/9#newcode231 > src/arm/assembler-thumb2-inl.h:231: pc_ += 2; // Padding > Can we emit a T2 nop here instead of leaving uninitialized memory? > fixed > > http://codereview.chromium.org/601028/diff/1/9#newcode274 > src/arm/assembler-thumb2-inl.h:274: Address target_pc = pc; > Here you are bringing the t2 file up to date wrt the ARM file. I would > have preferred to see this in a different change. > (see separate CL) > > http://codereview.chromium.org/601028/diff/1/10 > File src/arm/assembler-thumb2.cc (right): > > http://codereview.chromium.org/601028/diff/1/10#newcode34 > src/arm/assembler-thumb2.cc:34: // modified significantly by Google Inc. > Accidental edit. Removing the double 'above' would be better. > removed > > http://codereview.chromium.org/601028/diff/1/10#newcode597 > src/arm/assembler-thumb2.cc:597: // dataProcessingImm(op, s, rn, rd, > x.imm32_); > Commented out code. > removed > > http://codereview.chromium.org/601028/diff/1/7 > File src/arm/assembler-thumb2.h (right): > > http://codereview.chromium.org/601028/diff/1/7#newcode34 > src/arm/assembler-thumb2.h:34: // modified significantly by Google Inc. > Please revert this accidental change (above). > removed > > http://codereview.chromium.org/601028/diff/1/7#newcode50 > src/arm/assembler-thumb2.h:50: using namespace assembler::arm; > You can't use this here according to the style guide > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces Any suggestions? The problem are mainly the bxxxxx bit constants... Should I just duplicate them? Why are the assembler and the simulator in different namespaces? In particular, why is the assembler the only thing that is not in the asseembler::arm namespace? :) > http://codereview.chromium.org/601028/diff/1/7#newcode54 > src/arm/assembler-thumb2.h:54: H = 1 << 5, // halfword (or byte) > Please use correct punctuation in comments (initial capital and terminal > full stop) . Moved this stuff back to the original place in assembler-thumb.cc. > http://codereview.chromium.org/601028/diff/1/7#newcode55 > src/arm/assembler-thumb2.h:55: S6 = 1 << 6, // signed (or unsigned) > There seems to be a space too many in these lines (In the line above > it's after S6). > > http://codereview.chromium.org/601028/diff/1/7#newcode67 > src/arm/assembler-thumb2.h:67: RdMask = 15 << 12, // in str > instruction > Extra spaces (I might find it OK if it allowed the lines to line up, but > it doesn't). > > http://codereview.chromium.org/601028/diff/1/7#newcode943 > src/arm/assembler-thumb2.h:943: static InstrArm instrArm_at(byte* pc) { > return *reinterpret_cast<InstrArm*>(pc); } > We should be consistent with the existing style here. Since the > existing style in this originally non-Google file uses > lower_case_with_underscores then that is what the new code should use. > If we departed from that then the style guide should be followed, which > insists on UpperCamelCase for method names. > renamed to inst_arm_at > > http://codereview.chromium.org/601028/diff/1/7#newcode1043 > src/arm/assembler-thumb2.h:1043: bool thumbMode_; > lower_case_with_underscores_ > renamed to thumb_mode_ > > http://codereview.chromium.org/601028/diff/1/7#newcode1051 > src/arm/assembler-thumb2.h:1051: inline void emitThumb(InstrThumb x); > lower_case_with_underscores to be consistent with the preexisting code > in this class. See also other examples below. > > removed > http://codereview.chromium.org/601028/diff/1/7#newcode1084 > src/arm/assembler-thumb2.h:1084: > Please remove accidental edit. > > removed > http://codereview.chromium.org/601028 > -- Stefan Haustein Google UK Limited Registered Office: Belgrave House, 76 Buckingham Palace Road, London SW1W 9TQ; Registered in England Number: 3977902 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
