Please take another look.
https://codereview.chromium.org/16631002/diff/12001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):
https://codereview.chromium.org/16631002/diff/12001/src/arm/code-stubs-arm.cc#newcode7352
src/arm/code-stubs-arm.cc:7352: Handle<Map> cell_map(
On 2013/06/12 13:23:25, Michael Starzinger wrote:
nit: Should fit into one line now. Also, better use ...
Handle<Map> cell_map = masm->isolate()->factory()->cell_map();
Done.
https://codereview.chromium.org/16631002/diff/12001/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):
https://codereview.chromium.org/16631002/diff/12001/src/arm/stub-cache-arm.cc#newcode2860
src/arm/stub-cache-arm.cc:2860: __ ldr(scratch3(),
On 2013/06/12 13:23:25, Michael Starzinger wrote:
nit: Should fit into one line now.
Done.
https://codereview.chromium.org/16631002/diff/12001/src/arm/stub-cache-arm.cc#newcode2866
src/arm/stub-cache-arm.cc:2866: __ str(value(),
On 2013/06/12 13:23:25, Michael Starzinger wrote:
nit: Should fit into one line now.
Done.
https://codereview.chromium.org/16631002/diff/12001/src/assembler.h
File src/assembler.h (right):
https://codereview.chromium.org/16631002/diff/12001/src/assembler.h#newcode257
src/assembler.h:257: GLOBAL_PROPERTY_CELL,
On 2013/06/12 13:23:25, Michael Starzinger wrote:
Can we also rename this enum constant to "CELL"?
Done.
https://codereview.chromium.org/16631002/diff/12001/src/heap.cc
File src/heap.cc (right):
https://codereview.chromium.org/16631002/diff/12001/src/heap.cc#newcode392
src/heap.cc:392: PrintPID("JSGlobalPropertyCell space, used: %6"
V8_PTR_PREFIX "d KB"
On 2013/06/12 13:23:25, Michael Starzinger wrote:
Can we align this printf() format string so that it is aligned with
the others?
Otherwise the printout of --trace-gc-verbose looks ugly. This should
be easily
possible after the space has been renamed.
Done.
https://codereview.chromium.org/16631002/diff/12001/src/heap.cc#newcode6815
src/heap.cc:6815: new JSGlobalPropertyCellSpace(this,
max_old_generation_size_, CELL_SPACE);
On 2013/06/12 13:23:25, Michael Starzinger wrote:
The CELL_SPACE allocation id here is bogus.
Done.
https://codereview.chromium.org/16631002/diff/12001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):
https://codereview.chromium.org/16631002/diff/12001/src/ia32/code-stubs-ia32.cc#newcode7935
src/ia32/code-stubs-ia32.cc:7935: Handle<Map>
cell_map(masm->isolate()->heap()->cell_map());
On 2013/06/12 13:23:25, Michael Starzinger wrote:
See nit on other architecture.
Done.
https://codereview.chromium.org/16631002/diff/12001/src/mark-compact.cc
File src/mark-compact.cc (right):
https://codereview.chromium.org/16631002/diff/12001/src/mark-compact.cc#newcode3399
src/mark-compact.cc:3399: (JSGlobalPropertyCell::kValueOffset -
kHeapObjectTag);
On 2013/06/12 13:23:25, Michael Starzinger wrote:
Even though the values overlap I would recommend to use
Cell::kValueOffset here.
Done.
https://codereview.chromium.org/16631002/diff/12001/src/objects-visiting-inl.h
File src/objects-visiting-inl.h (right):
https://codereview.chromium.org/16631002/diff/12001/src/objects-visiting-inl.h#newcode248
src/objects-visiting-inl.h:248: void
StaticMarkingVisitor<StaticVisitor>::VisitGlobalPropertyCell(
On 2013/06/12 13:23:25, Michael Starzinger wrote:
I know it is probably a bit of additional work, but could we rename
these static
visitor methods to VisitCell as well? Alternatively we can also do
this in a
follow-up CL, in which case we should put in a TODO here or somewhere
in this
file.
Done.
https://codereview.chromium.org/16631002/diff/12001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/16631002/diff/12001/src/objects.cc#newcode1574
src/objects.cc:1574: accumulator->Add("JsGlobalPropertyCell for ");
On 2013/06/12 13:23:25, Michael Starzinger wrote:
nit: Let's at least drop the "Js" prefix in this printed string.
Done.
https://codereview.chromium.org/16631002/diff/12001/src/objects.h
File src/objects.h (left):
https://codereview.chromium.org/16631002/diff/12001/src/objects.h#oldcode8562
src/objects.h:8562: // [value]: value of the global property.
On 2013/06/12 13:23:25, Michael Starzinger wrote:
I actually like the comment lines and sometimes grep for "[foo]" in
the file,
can I get the comment line back? Or I could just learn to grep for
"DECL_ACCESSORS" but unfortunately we don't have that macro for int
accessors.
Done.
https://codereview.chromium.org/16631002/diff/12001/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/16631002/diff/12001/src/objects.h#newcode351
src/objects.h:351: V(CELL_TYPE)
\
On 2013/06/12 13:23:25, Michael Starzinger wrote:
Can we add the "Cell" type to the type hierarchy ASCII-art at the top
if this
file?
Done.
https://codereview.chromium.org/16631002/diff/12001/src/objects.h#newcode8602
src/objects.h:8602: void set_type(Type* value, WriteBarrierMode mode =
UPDATE_WRITE_BARRIER);
On 2013/06/12 13:23:25, Michael Starzinger wrote:
Can we use the DECL_ACCESSORS macro to declare this pair here?
No, unfortunately not. Note that they are not inlined. The problem is
that we would have type include type.h in objects-inl.h, and that's not
possible due to circular dependencies.
https://codereview.chromium.org/16631002/diff/12001/src/objects.h#newcode8620
src/objects.h:8620: kValueOffset,
On 2013/06/12 13:23:25, Michael Starzinger wrote:
nit: Should fit into three lines if we remove the first line-break.
Done.
https://codereview.chromium.org/16631002/diff/12001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):
https://codereview.chromium.org/16631002/diff/12001/src/x64/code-stubs-x64.cc#newcode6940
src/x64/code-stubs-x64.cc:6940: masm->isolate()->heap()->cell_map());
On 2013/06/12 13:23:25, Michael Starzinger wrote:
See nit for other architecture.
Done.
https://codereview.chromium.org/16631002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
