Detailed comments and nits.
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(
nit: Should fit into one line now. Also, better use ...
Handle<Map> cell_map = masm->isolate()->factory()->cell_map();
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(),
nit: Should fit into one line now.
https://codereview.chromium.org/16631002/diff/12001/src/arm/stub-cache-arm.cc#newcode2866
src/arm/stub-cache-arm.cc:2866: __ str(value(),
nit: Should fit into one line now.
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,
Can we also rename this enum constant to "CELL"?
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"
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.
https://codereview.chromium.org/16631002/diff/12001/src/heap.cc#newcode6815
src/heap.cc:6815: new JSGlobalPropertyCellSpace(this,
max_old_generation_size_, CELL_SPACE);
The CELL_SPACE allocation id here is bogus.
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());
See nit on other architecture.
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);
Even though the values overlap I would recommend to use
Cell::kValueOffset here.
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(
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.
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 ");
nit: Let's at least drop the "Js" prefix in this printed string.
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.
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.
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)
\
Can we add the "Cell" type to the type hierarchy ASCII-art at the top if
this file?
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);
Can we use the DECL_ACCESSORS macro to declare this pair here?
https://codereview.chromium.org/16631002/diff/12001/src/objects.h#newcode8620
src/objects.h:8620: kValueOffset,
nit: Should fit into three lines if we remove the first line-break.
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());
See nit for other architecture.
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.
