Mostly looking good. I think there might be an edge case with the final
element
in an iteration. And you've got some more handlification work. Other than
that,
I like the look of it!
https://codereview.chromium.org/236143002/diff/1/src/collection.js
File src/collection.js (right):
https://codereview.chromium.org/236143002/diff/1/src/collection.js#newcode130
src/collection.js:130: if (typeof f !== 'function') {
I think you want IS_SPEC_FUNCTION(f) here, which is a macro expanding
to:
%_ClassOf(arg) === 'Function'
https://codereview.chromium.org/236143002/diff/1/src/collection.js#newcode138
src/collection.js:138: %_CallFunction(receiver, entry.value,
entry.value, this, f);
Can you add a test that throws in its forEach function (both for set and
maps)?
https://codereview.chromium.org/236143002/diff/1/src/collection.js#newcode243
src/collection.js:243: throw
MakeTypeError('incompatible_method_receiver',
Can you add tests for this and the other error cases?
https://codereview.chromium.org/236143002/diff/1/src/collection.js#newcode247
src/collection.js:247: if (typeof f !== 'function') {
IS_SPEC_FUNCTION
https://codereview.chromium.org/236143002/diff/1/src/factory.cc
File src/factory.cc (right):
https://codereview.chromium.org/236143002/diff/1/src/factory.cc#newcode2005
src/factory.cc:2005:
InternalizeOneByteString(STATIC_ASCII_VECTOR("value")),
"value" and "done" are already available in the heap, and you can get a
handle for them from the factory by calling value_string() and
done_string() methods on this.
https://codereview.chromium.org/236143002/diff/1/src/objects-debug.cc
File src/objects-debug.cc (right):
https://codereview.chromium.org/236143002/diff/1/src/objects-debug.cc#newcode710
src/objects-debug.cc:710: CHECK(table()->IsOrderedHashTable() ||
table()->IsUndefined());
Seems like it would be nice to add some validation to the table
itself...maybe put a TODO here?
https://codereview.chromium.org/236143002/diff/1/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15772
src/objects.cc:15772:
table->set_iterators(table->GetHeap()->undefined_value());
Nit: s/table->GetHeap()/isolate->heap()/ (as above)
https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15804
src/objects.cc:15804: int new_capacity = 4;
This magic number probably means kMinCapacity in Allocate() should be
moved up to be a constant on the class.
https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15810
src/objects.cc:15810: for (Object* object = table->iterators();
Putting a DisallowHeapAllocation above this loop would be a good idea,
since you use a raw Object* to handle the iteration.
https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15854
src/objects.cc:15854: for (Object* object = table->iterators();
Same as above, DisallowHeapAllocation
https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15903
src/objects.cc:15903: for (Object* object = iterators();
And one more DisallowHeapAllocation.
https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15916
src/objects.cc:15916: new_iterator->set_table(this);
You triggered an allocation above, but are referring to a bare |this|
here, which is unsafe. CreateIterator has to be a static, and take the
table it's to operate on as a a handlified argument.
Would be nice to make this a lint error in V8...
https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15926
src/objects.cc:15926:
new_iterator->set_next_iterator(GetHeap()->undefined_value());
Nit: whitespace off here (extra space)
https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode16046
src/objects.cc:16046: table->NumberOfElements() +
table->NumberOfDeletedElements();
I see this a lot, maybe OrderedHashTable should expose an accessor that
returns the result of this expression.
https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode16090
src/objects.cc:16090: return factory->CreateIteratorResultObject(value,
false);
Method should be static since it does allocation.
https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode16090
src/objects.cc:16090: return factory->CreateIteratorResultObject(value,
false);
I notice you pass false here unconditionally, even if the table is
already Closed() at this point. Is that per spec, or is it up to the
implementation?
Also makes me wonder, what if the forEach function adds an entry to the
table during its final callback? It looks like that might cause the
forEach to terminate without hitting that last entry, and yet I think
the spec would say that the newly-added entry should also be iterated
over...
https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode16112
src/objects.cc:16112: ASSERT(kind == kKindValues || kind ==
kKindEntries);
I suppose it's arbitrary, but I was surprised at that kKindValues here
refers to the set's keys....
https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode16121
src/objects.cc:16121: Handle<FixedArray> array =
factory->NewFixedArray(2);
This method should be static since it does allocation.
https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode16160
src/objects.cc:16160: return factory->NewJSArrayWithElements(array);
Same as above, should be static.
https://codereview.chromium.org/236143002/diff/1/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/236143002/diff/1/src/objects.h#newcode4279
src/objects.h:4279: // [3]: live iterators (double linked list)
Nit: s/double linked/doubly-linked/
https://codereview.chromium.org/236143002/diff/1/src/objects.h#newcode4323
src/objects.h:4323: Handle<Iterator> CreateIterator(int kind);
This should at least have a comment referring to the enum it's supposed
to conform to.
Also, needs to be static.
https://codereview.chromium.org/236143002/diff/1/src/objects.h#newcode4326
src/objects.h:4326: return get(kIteratorsIndex);
Nit: if you can fit these on one line, please do so.
https://codereview.chromium.org/236143002/diff/1/src/runtime.cc
File src/runtime.cc (right):
https://codereview.chromium.org/236143002/diff/1/src/runtime.cc#newcode1580
src/runtime.cc:1580: || kind == JSSetIterator::kKindEntries);
Only one of these is used at the moment, I assume these are for when we
expose the iterator creation to JS? It would be nice to at least cover
these with tests, especially since I the kKindEntries versions are
currently (as written) buggy due to not being properly handlified.
https://codereview.chromium.org/236143002/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
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 v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
