Uploaded patch rebased to bleeding edge. The diffs show up other work so not
sure if i need to create a separate CL.
You need to rebase your change after
http://code.google.com/p/v8/source/detail?r=6448.
Done
Could you please take a look.
Thanks
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/26 11:36:37, antonm wrote:
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.
not required with the rebase.
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
On 2011/01/26 11:36:38, antonm wrote:
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
Done.
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).
On 2011/01/26 11:36:38, antonm wrote:
nit: refer <to> layout above?
Done.
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);
On 2011/01/26 11:36:38, antonm wrote:
we have handy LocalContext to get rid of those two lines (7241--7242)
Done.
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);
On 2011/01/26 11:36:38, antonm wrote:
we have handy v8_str for String::New
Done.
http://codereview.chromium.org/6170001/diff/119001/test/cctest/test-api.cc#newcode7250
test/cctest/test-api.cc:7250: v8::Handle<Value> value = CompileRun(
On 2011/01/26 11:36:38, antonm wrote:
nit: as you don't need return value, you can just omit it.
Done.
http://codereview.chromium.org/6170001/diff/119001/test/cctest/test-api.cc#newcode7252
test/cctest/test-api.cc:7252: " function inner (){ "
On 2011/01/26 11:36:38, antonm wrote:
nit: no space before ( in function declaration and a space before {
here and
below, please
Done.
http://codereview.chromium.org/6170001/diff/119001/test/cctest/test-api.cc#newcode7260
test/cctest/test-api.cc:7260: " outer()(); "
On 2011/01/26 11:36:38, antonm wrote:
why such involved structure: function which creates a closure?
simplified. on device test, the stub doesnt seem to move in initial few
gcs, so i have used 30 iterations (10 gcs) to ensure stub moves.
tracking gc for the stub address shows the initial gc forward logs show
the same address
forward 0x4086dba0 -> 0x4086dba0
forward 0x40875de0 -> 0x40875de0
forward 0x40877cc0 -> 0x40877cc0
forward 0x40875de0 -> 0x40875de0
forward 0x40877cc0 -> 0x408729a0 (address changed)
http://codereview.chromium.org/6170001/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
