Looking good. One round of comments, mostly nits.
https://codereview.chromium.org/236143002/diff/120001/src/bootstrapper.cc
File src/bootstrapper.cc (right):
https://codereview.chromium.org/236143002/diff/120001/src/bootstrapper.cc#newcode1370
src/bootstrapper.cc:1370:
ASSERT(object_function->initial_map()->inobject_properties() == 0);
nit: This assert belongs to the code creating the "iterator_result_map"
I think. It ensures that the "Object" map can be used as a base for
result object map. Can we move it down?
https://codereview.chromium.org/236143002/diff/120001/src/collection.js
File src/collection.js (right):
https://codereview.chromium.org/236143002/diff/120001/src/collection.js#newcode121
src/collection.js:121: var SET_ITERATOR_KIND_VALUES = 2;
nit: Can we move this constant, together with the comment into
macros.py?
https://codereview.chromium.org/236143002/diff/120001/src/collection.js#newcode130
src/collection.js:130: if (typeof f !== 'function') {
I think this should use IS_SPEC_FUNCTION for consistency with the rest
of the code. I'll defer to Andreas' comment about this.
https://codereview.chromium.org/236143002/diff/120001/src/collection.js#newcode238
src/collection.js:238: var MAP_ITERATOR_KIND_ENTRIES = 3;
nit: Likewise.
https://codereview.chromium.org/236143002/diff/120001/src/collection.js#newcode247
src/collection.js:247: if (typeof f !== 'function') {
Likewise.
https://codereview.chromium.org/236143002/diff/120001/src/factory.cc
File src/factory.cc (right):
https://codereview.chromium.org/236143002/diff/120001/src/factory.cc#newcode2000
src/factory.cc:2000: Handle<JSObject>
Factory::CreateIteratorResultObject(Handle<Object> value,
nit: Likewise, can we move this up to the other allocation functions?
https://codereview.chromium.org/236143002/diff/120001/src/factory.h
File src/factory.h (right):
https://codereview.chromium.org/236143002/diff/120001/src/factory.h#newcode596
src/factory.h:596: Handle<JSObject>
CreateIteratorResultObject(Handle<Object> value, bool done);
nit: Can we call this "NewIteratorResultObject" instead and move it up
to where the other allocation functions are?
https://codereview.chromium.org/236143002/diff/120001/src/objects-printer.cc
File src/objects-printer.cc (right):
https://codereview.chromium.org/236143002/diff/120001/src/objects-printer.cc#newcode775
src/objects-printer.cc:775: PrintF(out, " - count = ");
nit: I think there are couple of newline characters missing here. The
ShortPrint() will not print a trailing newline IIRC.
https://codereview.chromium.org/236143002/diff/120001/src/objects-visiting.h
File src/objects-visiting.h (right):
https://codereview.chromium.org/236143002/diff/120001/src/objects-visiting.h#newcode101
src/objects-visiting.h:101: V(JSMapIterator) \
These definitions should not be needed, the two objects behave like
normal JSObjects and don't need any special casing in the visitor.
https://codereview.chromium.org/236143002/diff/140001/src/objects-visiting.cc
File src/objects-visiting.cc (right):
https://codereview.chromium.org/236143002/diff/140001/src/objects-visiting.cc#newcode114
src/objects-visiting.cc:114: return kVisitJSSetIterator;
We should be able to use a generic visitor for JSObjects here.
https://codereview.chromium.org/236143002/
--
--
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.
