Great work, thanks. Almost LGTM---you need to rebase your change after
http://code.google.com/p/v8/source/detail?r=6448.
Thanks again.
http://codereview.chromium.org/6170001/diff/90001/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):
http://codereview.chromium.org/6170001/diff/90001/src/arm/macro-assembler-arm.cc#newcode547
src/arm/macro-assembler-arm.cc:547: int pending_pushes = stack_space +
4; // 4 pushes in this function.
On 2011/01/24 09:43:31, zaheer wrote:
On 2011/01/21 17:56:36, antonm wrote:
> May you explain: it looks like only pending_pushes % 2 are used in
this
function
> which looks odd and this + 4 addition looks strange as well. What
do I miss?
sorry i miss your comment.
>
> BTW, do we have tests for API function invocations with many (> 3)
arguments?
the call api requests space for 4 arguments (Arguments::implicit_args,
values,
length, is_construct) and existing tests e.g.
(FastApiCallback_SimpleSignature)
already cover this case.
Maybe i miss your comment.
I am sorry, I meant it's somewhat strange to see pending_pushes % 2 ==
(stack_space + 4) % 2 == stack_space % 2 + 4 % 2 == stack_space % 2.
Overall, may I ask you to get rid of pending_pushes altogether and
instead have something like:
// If stack is unaligned....
Condition condition = stack_space % 2 == 0 ? ne : eq;
push(r7, condition)
or
// If stack is unaligned...
if (stack_space % 2 == 0) {
push(r7, ne);
} ...
If you prefer to keep constant 4 to make it more obvious how to change
this function later, may you introduced named constant and once again
write something like
if ((stack_space + kEnterExitFramePushes) % 2)
And regarding tests: I meant API function which accept many argument,
not the Arguments structure, but it's not relevant here, sorry for
noise.
http://codereview.chromium.org/6170001/diff/90001/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):
http://codereview.chromium.org/6170001/diff/90001/src/arm/simulator-arm.cc#newcode1590
src/arm/simulator-arm.cc:1590: SimulatorRuntimeApiCall target =
On 2011/01/24 09:43:31, zaheer wrote:
On 2011/01/21 17:56:36, antonm wrote:
> please, consider refactoring the common logic of logging and stack
alignment
> checks
The log is unique to each case, the stack alignment check refactoring
also has a
problem - moving it down will mean it will print after executing
function,
moving it before will impact readability of log. Any ideas.
Ok, let's leave it as you have it now.
http://codereview.chromium.org/6170001/diff/90001/src/heap.h
File src/heap.h (right):
http://codereview.chromium.org/6170001/diff/90001/src/heap.h#newcode121
src/heap.h:121: #if V8_TARGET_ARCH_ARM
Nice, thanks.
On 2011/01/24 09:43:31, zaheer wrote:
On 2011/01/21 17:56:36, antonm wrote:
> Maybe we should restructure that differently:
>
> I'd prefer to have a single STRONG_ROOT_LIST_ARM macro which could
be empty,
or
> keep DirectCEntryStub, or both.
>
> The code could look like
>
> #if V8_TARGET_ARCH_ARM && !V8_INTERPRETED_REGEXP
> ...
> #elif V8_TARGET_ARCH_ARM
> #else
> #endif
>
> What do you think?
Agree. i simplified one more bit and removed STRONG_ROOT_LIST_ARM.
#if V8_TARGET_ARCH_ARM && !V8_INTERPRETED_REGEXP
#define STRONG_ROOT_LIST(V)
\
UNCONDITIONAL_STRONG_ROOT_LIST(V)
\
V(Code, re_c_entry_code, RegExpCEntryCode)
\
V(Code, direct_c_entry_code, DirectCEntryCode)
#elif V8_TARGET_ARCH_ARM
#define STRONG_ROOT_LIST(V)
\
UNCONDITIONAL_STRONG_ROOT_LIST(V)
\
V(Code, direct_c_entry_code, DirectCEntryCode)
#else
#define STRONG_ROOT_LIST(V)
\
UNCONDITIONAL_STRONG_ROOT_LIST(V)
#endif
Is this ok?
http://codereview.chromium.org/6170001/diff/119001/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):
http://codereview.chromium.org/6170001/diff/119001/src/arm/simulator-arm.cc#newcode1601
src/arm/simulator-arm.cc:1601: // builtin/runtime call
nit: add a dot please.
v8 style is to end comments with a dot when they start as the statement
indent and leave no end if they are inline:
// Foo.
bar(); // xyzzy
http://codereview.chromium.org/6170001/diff/119001/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):
http://codereview.chromium.org/6170001/diff/119001/src/arm/stub-cache-arm.cc#newcode624
src/arm/stub-cache-arm.cc:624: // r2 points to call data as expected by
Arguments class (refer layout above).
nit: refer <to> layout above?
http://codereview.chromium.org/6170001/diff/119001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):
http://codereview.chromium.org/6170001/diff/119001/test/cctest/test-api.cc#newcode7242
test/cctest/test-api.cc:7242: v8::Context::Scope context_scope(context);
we have handy LocalContext to get rid of those two lines (7241--7242)
http://codereview.chromium.org/6170001/diff/119001/test/cctest/test-api.cc#newcode7246
test/cctest/test-api.cc:7246:
context->Global()->Set(String::New("nativeobject"), nativeobject_obj);
we have handy v8_str for String::New
http://codereview.chromium.org/6170001/diff/119001/test/cctest/test-api.cc#newcode7250
test/cctest/test-api.cc:7250: v8::Handle<Value> value = CompileRun(
nit: as you don't need return value, you can just omit it.
http://codereview.chromium.org/6170001/diff/119001/test/cctest/test-api.cc#newcode7252
test/cctest/test-api.cc:7252: " function inner (){ "
nit: no space before ( in function declaration and a space before { here
and below, please
http://codereview.chromium.org/6170001/diff/119001/test/cctest/test-api.cc#newcode7260
test/cctest/test-api.cc:7260: " outer()(); "
why such involved structure: function which creates a closure?
http://codereview.chromium.org/6170001/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
