Bug 175653 – Speculatively change iteration protocall to use the same next function

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

nSpeculatively change iteration protocall to use the same next function

[Keith Miller]

Created attachment 318323 [details]
Patch

[Saam Barati]

Comment on attachment 318323 [details]
Patch

You need to also change builtins/IteratorHelpers.js.
Maybe other places as well?

Doesn't this break some of our tests?

[Keith Miller]

Created attachment 318386 [details]
Patch

[Build Bot]

Attachment 318386 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/IteratorOperations.h:40:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 318386 [details]
Patch

Attachment 318386 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4331620

New failing tests:
js/sequence-iterator-protocol-2.html

[Build Bot]

Created attachment 318394 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 318386 [details]
Patch

Attachment 318386 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4331621

New failing tests:
js/sequence-iterator-protocol-2.html

[Build Bot]

Created attachment 318395 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 318386 [details]
Patch

Attachment 318386 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4331597

New failing tests:
js/sequence-iterator-protocol-2.html

[Build Bot]

Created attachment 318397 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 318386 [details]
Patch

Attachment 318386 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4332141

New failing tests:
js/sequence-iterator-protocol-2.html

[Build Bot]

Created attachment 318415 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Sam Weinig]

What is the goal of making this change?

[Keith Miller]

(In reply to Sam Weinig from comment #13)
> What is the goal of making this change?

It's related to https://bugs.webkit.org/show_bug.cgi?id=175454, as it will simplify the implementation of that. Also, it's a possible spec change in the next version of ES6. I'm landing the patch now to see if it breaks any pages in STP, I highly doubt it will though.

[Keith Miller]

Created attachment 318533 [details]
Patch

[Build Bot]

Attachment 318533 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/IteratorOperations.h:40:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 318533 [details]
Patch

You still need to change the builtin file that does iteration for spread

[Keith Miller]

Created attachment 318685 [details]
Patch

[Saam Barati]

Comment on attachment 318685 [details]
Patch

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

r=me

Please add tests that verify "next" is only accessed once per for-of, spread, and anything else that does the protocol (not sure if there is anything else off the top of my head)

> Source/JavaScriptCore/ChangeLog:13
> +        See: https://github.com/tc39/ecma262/issues/976

Let's also have a bugzilla that tracks if you haven't already made one.

[GSkachkov]

Comment on attachment 318685 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:19
> +        (JSC::BytecodeGenerator::emitIteratorNext):

There is might be conflict after landing async iteration : https://trac.webkit.org/r221080, please do rebase.

[Keith Miller]

Created attachment 321608 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 321608 [details]
Patch for landing

Clearing flags on attachment: 321608

Committed r222421: <http://trac.webkit.org/changeset/222421>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

It looks like this is causing test262 timeouts:
https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20Test262%20%28Tests%29/builds/4314

Locally I'm seeing this test hang forever:
./test262/test/language/statements/for-of/iterator-next-reference.js

[Keith Miller]

(In reply to Joseph Pecoraro from comment #24)
> It looks like this is causing test262 timeouts:
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20Release%20Test262%20%28Tests%29/builds/4314
> 
> Locally I'm seeing this test hang forever:
> ./test262/test/language/statements/for-of/iterator-next-reference.js

Ah, I must have missed that test for some reason... I put a fix here: https://bugs.webkit.org/show_bug.cgi?id=177412

[Radar WebKit Bug Importer]

<rdar://problem/34694460>