https://codereview.chromium.org/237093006/diff/40001/src/factory.cc
File src/factory.cc (right):
https://codereview.chromium.org/237093006/diff/40001/src/factory.cc#newcode1587
src/factory.cc:1587: Heap* heap = isolate()->heap();
On 2014/04/15 09:54:59, Michael Starzinger wrote:
nit: There should be Factory::heap() IIRC, I think there is no need to
pull out
the Heap into a local variable if we use that one.
There's no heap() method. Lets address this in a separate CL.
https://codereview.chromium.org/237093006/diff/40001/src/factory.cc#newcode1589
src/factory.cc:1589: heap->InitializeJSObjectFromMap(*jsobj,
FixedArray::cast(*properties), *map);
On 2014/04/15 09:54:59, Michael Starzinger wrote:
nit: No need to cast the FixedArray anymore, it is also typed
correctly.
Done.
https://codereview.chromium.org/237093006/diff/40001/src/factory.cc#newcode1600
src/factory.cc:1600: InitializeFunction(js_function, shared,
the_hole_value());
On 2014/04/15 09:54:59, Michael Starzinger wrote:
Same comment as below goes for the call to InitializeFunction. This
whole method
needs to be re-ordered to maintain a consistent heap at allocation
points.
Done.
https://codereview.chromium.org/237093006/diff/40001/src/factory.cc#newcode1606
src/factory.cc:1606: heap->CreateFillerObjectAt(
On 2014/04/15 09:54:59, Michael Starzinger wrote:
Creating the filler object needs to happen right after we change the
map of the
object, otherwise the heap is no longer iterable. Hence the
allocations in the
block above could cause GCs that see an inconsistent heap. Please move
this up
to after the call to InitializeJSObjectFromMap above.
I fixed this in a little bit different way than you suggested.
https://codereview.chromium.org/237093006/diff/40001/src/factory.cc#newcode1625
src/factory.cc:1625: isolate()->factory()->NewFixedArray(prop_size,
TENURED);
On 2014/04/15 09:54:59, Michael Starzinger wrote:
nit: We are already inside the Factory, just call NewFixedArray
directly here.
Fixed here and in all other places in the file.
https://codereview.chromium.org/237093006/diff/40001/src/factory.cc#newcode1630
src/factory.cc:1630: Heap* heap = isolate()->heap();
On 2014/04/15 09:54:59, Michael Starzinger wrote:
nit: There should be Factory::heap() IIRC, I think there is no need to
pull out
the Heap into a local variable if we use that one.
Done.
https://codereview.chromium.org/237093006/diff/40001/src/factory.cc#newcode1632
src/factory.cc:1632: heap->InitializeJSObjectFromMap(*object,
FixedArray::cast(*properties), *map);
On 2014/04/15 09:54:59, Michael Starzinger wrote:
nit: No need to cast the FixedArray anymore, it is also typed
correctly.
Done.
https://codereview.chromium.org/237093006/diff/40001/src/factory.h
File src/factory.h (right):
https://codereview.chromium.org/237093006/diff/40001/src/factory.h#newcode403
src/factory.h:403: Handle<JSFunction> constructor, Handle<JSGlobalProxy>
global);
On 2014/04/15 09:54:59, Michael Starzinger wrote:
suggestion: I know it was like this before your change, but I think it
might
make more sense to have "global" be the first argument. WDYT?
I agree, done.
https://codereview.chromium.org/237093006/diff/40001/src/factory.h#newcode602
src/factory.h:602: // Creates a heap object based on the map.
On 2014/04/15 09:54:59, Michael Starzinger wrote:
nit: Can we add one sentence saying that the fields of the heap object
are not
initialized by this function? It's the responsibility of the caller to
do that.
Done.
https://codereview.chromium.org/237093006/
--
--
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/d/optout.
