http://codereview.chromium.org/3792003/diff/1/3
File src/api.cc (right):
http://codereview.chromium.org/3792003/diff/1/3#newcode461
src/api.cc:461: : prev_next_(i::HandleScope::current_.next)
On 2010/10/14 12:03:21, antonm wrote:
nit: I think v8 style mandates comma at the end of initialization
Done.
http://codereview.chromium.org/3792003/diff/1/3#newcode477
src/api.cc:477: i::HandleScope::current_.level--;
On 2010/10/14 12:03:21, antonm wrote:
ASSERT(current_.level >= 0)?
Done.
http://codereview.chromium.org/3792003/diff/1/4
File src/api.h (right):
http://codereview.chromium.org/3792003/diff/1/4#newcode471
src/api.h:471: internal::Object** block_limit =
&block_start[kHandleBlockSize];
On 2010/10/14 12:03:21, antonm wrote:
why not block_start + kHandleBlockSize?
Done.
http://codereview.chromium.org/3792003/diff/1/9
File src/ia32/code-stubs-ia32.cc (right):
http://codereview.chromium.org/3792003/diff/1/9#newcode3115
src/ia32/code-stubs-ia32.cc:3115:
ExternalReference::handle_scope_next_address();
On 2010/10/14 12:03:21, antonm wrote:
nit: I think you need 4 spaces alignment here
Done.
http://codereview.chromium.org/3792003/diff/1/9#newcode3124
src/ia32/code-stubs-ia32.cc:3124: __
inc(Operand::StaticVariable(level_address));
On 2010/10/14 12:03:21, antonm wrote:
inc/dec should be avoided as per Intel Optimization Guide
Changed to add/sub
http://codereview.chromium.org/3792003/diff/1/9#newcode3131
src/ia32/code-stubs-ia32.cc:3131: __
dec(Operand::StaticVariable(level_address));
On 2010/10/14 12:03:21, antonm wrote:
Please, add a debug check that level is >= 0
Done.
http://codereview.chromium.org/3792003/diff/1/9#newcode3158
src/ia32/code-stubs-ia32.cc:3158: // Finish restoring previous handle
scope.
On 2010/10/14 12:03:21, antonm wrote:
I don't think we should have this complicated control flow: if
exception has
been thrown, we're already on a slow path and two move instructions
won't save
us.
Done.
http://codereview.chromium.org/3792003/diff/1/11
File src/ia32/macro-assembler-ia32.h (left):
http://codereview.chromium.org/3792003/diff/1/11#oldcode483
src/ia32/macro-assembler-ia32.h:483: void PushHandleScope(Register
scratch);
On 2010/10/14 12:03:21, antonm wrote:
why those macros are removed? I'd prefer if new logic would be
implemented in
macros (see also comments about implementation), .e.g, we could reuse
it to
invoke other API stuff from generated code.
As we discussed I introduced TailCallApiFunction macro.
http://codereview.chromium.org/3792003/diff/1/16
File src/x64/code-stubs-x64.cc (right):
http://codereview.chromium.org/3792003/diff/1/16#newcode2544
src/x64/code-stubs-x64.cc:2544: __ incl(Operand(base_reg,
kLevelOffset));
On 2010/10/14 12:03:21, antonm wrote:
ditto for incl/decl
Done.
http://codereview.chromium.org/3792003/diff/1/16#newcode2550
src/x64/code-stubs-x64.cc:2550: __ movq(rsi, rdx);
On 2010/10/14 12:03:21, antonm wrote:
apparently rsi a.k.a. base_reg is clobbered here, so why you don't
need to
restore it below (see ln. 2563)?
It's clobbered unless _WIN64. In this case it's restored there. So it's
OK.
http://codereview.chromium.org/3792003/diff/1/16#newcode2562
src/x64/code-stubs-x64.cc:2562: // rsi is only callee save on Win64. On
other platforms it must be restored.
On 2010/10/14 12:03:21, antonm wrote:
nit: save -> save[d]
They named in such a way in docs ("Callee-save registers are registers
that you have to
save before using them and restore after using them (also called
non-volatile registers)"). So let's be consistent.
http://codereview.chromium.org/3792003/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
