[webkit-reviews] review granted: [Bug 221526] [JSC] C++ iteration should support fast iterator protocol : [Attachment 419536] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Feb 9 15:47:00 PST 2021


Alexey Shvayka <shvaikalesh at gmail.com> has granted Yusuke Suzuki
<ysuzuki at apple.com>'s request for review:
Bug 221526: [JSC] C++ iteration should support fast iterator protocol
https://bugs.webkit.org/show_bug.cgi?id=221526

Attachment 419536: Patch

https://bugs.webkit.org/attachment.cgi?id=419536&action=review




--- Comment #8 from Alexey Shvayka <shvaikalesh at gmail.com> ---
Comment on attachment 419536
  --> https://bugs.webkit.org/attachment.cgi?id=419536
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419536&action=review

Well done! r=me with a comment on test and a few nits.

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:867
> +    if (prepareForFastArrayIteration(vm, globalObject, iterable,
symbolIterator) == IterationMode::FastArray) {

nit: "prepare" in the name suggests the method is performing some side-effects,
yet it's pure. Maybe it's worth renaming it to something like
"getIterationMode"?

> Source/JavaScriptCore/runtime/IteratorOperations.cpp:249
> +    // We don't want to allocate the values function just to check if it's
the same as our function at so we use the concurrent accessor.

typo: extra "at" after "function"

> Source/JavaScriptCore/runtime/IteratorOperations.cpp:264
> +    // FIXME: We want to support broader JSArrays so long as
array[@@iterator] is not defined.

typo: *as* long as

> Source/JavaScriptCore/runtime/IteratorOperations.h:70
> +	       JSValue nextValue = array->getIndex(globalObject,
static_cast<unsigned>(index));

nit: Do we really need the static_cast<unsigned> here given that `index` is
unsigned and getIndex() has only one overload.

> Source/JavaScriptCore/runtime/IteratorOperations.h:76
> +		  
iterator->internalField(JSArrayIterator::Field::Index).setWithoutWriteBarrier(j
sNumber(index + 1));

This is very tricky, great catch! Could you please expand
stress/map-constructor-hole-throw.js to cover this line?
Since we get a hold of iterator whose [[IteratedObject]] is still defined, we
can test its [[ArrayIteratorNextIndex]] value by calling next(), right?

> Source/JavaScriptCore/runtime/IteratorOperations.h:112
> +	       JSValue nextValue = array->getIndex(&globalObject,
static_cast<unsigned>(index));

Ditto on static_cast<unsigned>.


More information about the webkit-reviews mailing list