I'll look into changing things to use static next
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') {
On 2014/04/14 19:15:42, adamk wrote:
I think you want IS_SPEC_FUNCTION(f) here, which is a macro expanding
to:
%_ClassOf(arg) === 'Function'
IS_SPEC_FUNCTION is not correct any more. It tests that the instance is
a true function. What we really care about is Callable. Most of the
IS_SPEC function have been removed in the spec and now the spec checks
for Callable instead.
https://codereview.chromium.org/236143002/diff/1/src/collection.js#newcode138
src/collection.js:138: %_CallFunction(receiver, entry.value,
entry.value, this, f);
On 2014/04/14 19:15:42, adamk wrote:
Can you add a test that throws in its forEach function (both for set
and maps)?
Done.
https://codereview.chromium.org/236143002/diff/1/src/collection.js#newcode243
src/collection.js:243: throw
MakeTypeError('incompatible_method_receiver',
On 2014/04/14 19:15:42, adamk wrote:
Can you add tests for this and the other error cases?
Done.
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")),
On 2014/04/14 19:15:42, adamk wrote:
"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.
Done.
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());
On 2014/04/14 19:15:42, adamk wrote:
Seems like it would be nice to add some validation to the table
itself...maybe
put a TODO here?
Done.
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());
On 2014/04/14 19:15:42, adamk wrote:
Nit: s/table->GetHeap()/isolate->heap()/ (as above)
Done.
https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15804
src/objects.cc:15804: int new_capacity = 4;
On 2014/04/14 19:15:42, adamk wrote:
This magic number probably means kMinCapacity in Allocate() should be
moved up
to be a constant on the class.
Done.
https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15810
src/objects.cc:15810: for (Object* object = table->iterators();
On 2014/04/14 19:15:42, adamk wrote:
Putting a DisallowHeapAllocation above this loop would be a good idea,
since you
use a raw Object* to handle the iteration.
Done.
https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15854
src/objects.cc:15854: for (Object* object = table->iterators();
On 2014/04/14 19:15:42, adamk wrote:
Same as above, DisallowHeapAllocation
Done.
https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15903
src/objects.cc:15903: for (Object* object = iterators();
On 2014/04/14 19:15:42, adamk wrote:
And one more DisallowHeapAllocation.
Done.
https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode15926
src/objects.cc:15926:
new_iterator->set_next_iterator(GetHeap()->undefined_value());
On 2014/04/14 19:15:42, adamk wrote:
Nit: whitespace off here (extra space)
Done.
https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode16046
src/objects.cc:16046: table->NumberOfElements() +
table->NumberOfDeletedElements();
On 2014/04/14 19:15:42, adamk wrote:
I see this a lot, maybe OrderedHashTable should expose an accessor
that returns
the result of this expression.
Any suggestion for a name?
https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode16090
src/objects.cc:16090: return factory->CreateIteratorResultObject(value,
false);
On 2014/04/14 19:15:42, adamk wrote:
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...
I think you are right. I'll review the spec and add tests.
https://codereview.chromium.org/236143002/diff/1/src/objects.cc#newcode16112
src/objects.cc:16112: ASSERT(kind == kKindValues || kind ==
kKindEntries);
On 2014/04/14 19:15:42, adamk wrote:
I suppose it's arbitrary, but I was surprised at that kKindValues here
refers to
the set's keys....
This follows from the spec. Set.prototype has values but not keys. We
could internally only use keys but either way it is pretty arbitrary.
I added a comment.
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)
On 2014/04/14 19:15:42, adamk wrote:
Nit: s/double linked/doubly-linked/
Done.
https://codereview.chromium.org/236143002/diff/1/src/objects.h#newcode4323
src/objects.h:4323: Handle<Iterator> CreateIterator(int kind);
On 2014/04/14 19:15:42, adamk wrote:
This should at least have a comment referring to the enum it's
supposed to
conform to.
Also, needs to be static.
This is an instance method. The iterator depends on the
OrderedHashTable.
I added comment clarifying this. Maybe we should provide some other API
to make this more explicit?
https://codereview.chromium.org/236143002/diff/1/src/objects.h#newcode4326
src/objects.h:4326: return get(kIteratorsIndex);
On 2014/04/14 19:15:42, adamk wrote:
Nit: if you can fit these on one line, please do so.
Done.
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);
On 2014/04/14 19:15:42, adamk wrote:
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.
Entries is being tested for Map. I can expand the cctest?
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.
