you can check in. On Thu, Sep 11, 2008 at 5:18 PM, <[EMAIL PROTECTED]> wrote: > First set of replies... > > > http://codereview.chromium.org/1930/diff/1/9 > File src/builtins-arm.cc (right): > > http://codereview.chromium.org/1930/diff/1/9#newcode67 > Line 67: __ push(r1); // constructor function > On 2008/09/11 21:33:13, Feng Qian wrote: >> >> The constructor function pushed on the stack seems not used. Double > > check! > > It is used for example on line 78: > __ ldr(r1, MemOperand(sp, kPointerSize)); > > http://codereview.chromium.org/1930/diff/1/9#newcode100 > Line 100: // r3: number of arguments (smi-tagged) > On 2008/09/11 21:33:13, Feng Qian wrote: >> >> How about adding one more line comment here: >> // r2: caller sp > > Fixed > > http://codereview.chromium.org/1930/diff/1/18 > File src/codegen-arm.cc (right): > > http://codereview.chromium.org/1930/diff/1/18#newcode3876 > Line 3876: ASSERT(args->length() == 1); > On 2008/09/11 21:33:13, Feng Qian wrote: >> >> Please update comment above function declaration. > > Outdated comment removed. > > http://codereview.chromium.org/1930/diff/1/8 > File src/frames-arm.cc (right): > > http://codereview.chromium.org/1930/diff/1/8#newcode48 > Line 48: return > static_cast<StackFrame::Type>(Smi::cast(marker)->value()); > On 2008/09/11 21:33:13, Feng Qian wrote: >> >> Now this function looks exactly like the on in frame-ia32.cc. Can we > > move it to >> >> frames.cc? > > This change is already unwieldy. Some of the cleanup should move to a > follow up change. > > http://codereview.chromium.org/1930/diff/1/20 > File src/macro-assembler-arm.cc (right): > > http://codereview.chromium.org/1930/diff/1/20#newcode299 > Line 299: > On 2008/09/11 21:33:13, Feng Qian wrote: >> >> This part is a bit complicated. I can see unnecessary mov instructions > > in >> >> different paths. For example, when both actual and expected are > > immediate but >> >> not equal, there are two instructions that move actual.immediate() > > into r0. >> > > Removed leftover code from the old calling convention. Thanks for > spotting! > > http://codereview.chromium.org/1930/diff/1/20#newcode665 > Line 665: Builtins::JavaScript id, bool* resolved) { > On 2008/09/11 21:33:13, Feng Qian wrote: >> >> Can this function be made that same as in IA32? (non-static). >> >> Part of code looks identical as the same function in IA32. You may > > want to >> >> refactor the common code out. > > Factored the common code out into Builtins::GetCode(JavaScript id, bool* > resolved). > > http://codereview.chromium.org/1930/diff/1/13 > File src/simulator-arm.cc (right): > > http://codereview.chromium.org/1930/diff/1/13#newcode94 > Line 94: OS::Abort(); > On 2008/09/11 10:43:23, Kasper Lund wrote: >> >> Is this intentional? > > Yes, this was just needed so that test.py does not wait until the > timeout expires when hitting a stop instruction. Will be removed before > putback. > > http://codereview.chromium.org/1930 >
--~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
