Thanks, Kevin. Good suggestions.
Since both Lasse and you LGTM'd this I will plan to land this as soon as the
change this depends on (http://codereview.chromium.org/6694044/) lands.
Cheers!
Martin
http://codereview.chromium.org/6698015/diff/6001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):
http://codereview.chromium.org/6698015/diff/6001/src/arm/code-stubs-arm.cc#newcode4860
src/arm/code-stubs-arm.cc:4860:
STATIC_ASSERT(Heap::arguments_callee_index == 1);
On 2011/03/16 09:48:41, Kevin Millikin wrote:
That name is old school.
You might as well change it to kArgumentsCalleeIndex as part of this
change.
Done.
http://codereview.chromium.org/6698015/diff/6001/src/arm/code-stubs-arm.cc#newcode4862
src/arm/code-stubs-arm.cc:4862: MemOperand callee_operand =
FieldMemOperand(
On 2011/03/16 09:48:41, Kevin Millikin wrote:
You're right, this is ugly. I would write instead:
const int kCalleeOffset =
JSObject::kHeaderSize + Heap::kArgumentsCalleeIndex *
kPointerSize;
__ str(r3, FieldMemOperand(r0, kCalleeOffset));
Done. Good suggestion!
http://codereview.chromium.org/6698015/diff/6001/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):
http://codereview.chromium.org/6698015/diff/6001/src/arm/codegen-arm.cc#newcode605
src/arm/codegen-arm.cc:605: ? ArgumentsAccessStub::NEW_OBJECT_STRICT
On 2011/03/16 09:48:41, Kevin Millikin wrote:
OBJECT is just noise. How about NEW_STRICT and NEW_NON_STRICT?
Done.
http://codereview.chromium.org/6698015/diff/6001/src/code-stubs.h
File src/code-stubs.h (right):
http://codereview.chromium.org/6698015/diff/6001/src/code-stubs.h#newcode684
src/code-stubs.h:684: }
On 2011/03/16 09:48:41, Kevin Millikin wrote:
On 2011/03/16 01:21:24, Martin Maly wrote:
> This is tragic. Using ?: results in linker not being able to find
the
> kArguments* constants. Unless I am missing something it smells like
gcc bug...
> so I had to resort to if .. else to work around it. It didn't make
difference
> whether the code was in inline method or not.
It looks like it's declared in heap.h, but there is no definition in
heap.cc.
We get away with that all the time, but the compiler doesn't have to
allow it.
Fix is to add:
const int Heap::kArgumentsObjectSizeStruct;
const int Heap::kArgumentsObjectSize;
in heap.cc.
(Relevant part of the standard:
9.4.2.4 4 If a static data member is of const integral or const
enumeration
type, its declaration in the class definition can specify a
constant-initializer
which shall be an integral constant expression (_expr.const_). In
that case,
the member can appear in integral constant expressions within its
scope. The
member shall still be defined in a namespace scope if it is used in
the program
and the namespace scope definition shall not contain an initializer.)
Done. Thanks for the detailed info!
http://codereview.chromium.org/6698015/diff/6001/src/contexts.h
File src/contexts.h (right):
http://codereview.chromium.org/6698015/diff/6001/src/contexts.h#newcode90
src/contexts.h:90: V(ARGUMENTS_BOILERPLATE_STRICT_INDEX, JSObject, \
On 2011/03/16 09:48:41, Kevin Millikin wrote:
Probably reads better as STRICT_ARGUMENTS_BOILERPLATE_INDEX, JSObject,
strict_arguments_boilerplate
Done. Using STRICT_MODE_ARGUMENTS_BOILERPLATE_INDEX which is consistent
with what Mads recommended for the other names.
http://codereview.chromium.org/6698015/diff/6001/src/heap.h
File src/heap.h (right):
http://codereview.chromium.org/6698015/diff/6001/src/heap.h#newcode593
src/heap.h:593: static const int arguments_length_index = 0;
On 2011/03/16 09:48:41, Kevin Millikin wrote:
kArgumentsLengthIndex
Done.
http://codereview.chromium.org/6698015/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
