Addressed most of the comments. Will now work on improving test coverage.
https://chromiumcodereview.appspot.com/10908194/diff/3001/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):
https://chromiumcodereview.appspot.com/10908194/diff/3001/src/arm/lithium-codegen-arm.cc#newcode468
src/arm/lithium-codegen-arm.cc:468: int* args_index,
On 2012/09/12 07:14:22, Sven Panne wrote:
Nit: args_foo => arguments_foo for consistency here and at similar
places.
Done.
https://chromiumcodereview.appspot.com/10908194/diff/3001/src/arm/lithium-codegen-arm.h
File src/arm/lithium-codegen-arm.h (right):
https://chromiumcodereview.appspot.com/10908194/diff/3001/src/arm/lithium-codegen-arm.h#newcode152
src/arm/lithium-codegen-arm.h:152: int* arguments_index,
On 2012/09/12 07:14:22, Sven Panne wrote:
I think later it will be very hard to understand what arguments_index
actually
is, so we should document this somehow, but I am not sure where.
Perhaps arguments_offset might be a better name? I'll leave this up to
you, but
we need some kind of comment in any case.
Done. I added the documentation into LCodeGen::WriteTranslation where
it's actually used. About the name, I would still like to keep it named
index, because it is an operand index (same as LOperand::index) and
follows the same rules. That's in sync with the usage of "index"
throughout lithium.
https://chromiumcodereview.appspot.com/10908194/diff/3001/src/deoptimizer.cc
File src/deoptimizer.cc (right):
https://chromiumcodereview.appspot.com/10908194/diff/3001/src/deoptimizer.cc#newcode679
src/deoptimizer.cc:679: // Materialize all heap numbers before looking
at arguments.
On 2012/09/12 07:14:22, Sven Panne wrote:
Describe why this is necessary to do before arguments objects
materialization.
Done.
https://chromiumcodereview.appspot.com/10908194/diff/3001/src/deoptimizer.cc#newcode718
src/deoptimizer.cc:718: if (!frame->has_adapted_arguments()) {
On 2012/09/12 07:14:22, Sven Panne wrote:
Can rearrange things a bit here so that we have a single 'if'?
if (frame->has_adapted_arguments()) {
...
} else {
...
}
Seeing the different cases is a bit hard currently, but this might be
due to the
inherent complexity of the problem. :-)
Done. I removed the static helper method completely and merged it into
this method. It is now clearer when we use the output frame and when we
use the deoptimization info to materialize the arguments object.
https://chromiumcodereview.appspot.com/10908194/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
