Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Normative: add Iterator Helpers #3395

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Normative: add Iterator Helpers #3395

wants to merge 15 commits into from
+545 −21

Conversation

michaelficarra
Copy link
Member

@michaelficarra michaelficarra added normative change Affects behavior required to correctly evaluate some ECMAScript source text pending stage 4 This proposal has not yet achieved stage 4, but may otherwise be ready to merge. proposal This is related to a specific proposal, and will be closed/merged when the proposal reaches stage 4. labels Aug 16, 2024
@michaelficarra michaelficarra mentioned this pull request Aug 16, 2024
Closed
14 tasks
michaelficarra
michaelficarra commented Aug 16, 2024
View reviewed changes
spec.html
@@ -6956,6 +6996,28 @@ <h1>
</emu-alg>
</emu-clause>

<emu-clause id="sec-getiteratorflattenable" type="abstract operation">
<h1>
GetIteratorFlattenable (
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to come up with a better name for this AO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment was about GetIteratorFlattenable (it's no longer displaying under the appropriate line in the diff view).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks right on the conversation view, at least

@jmdyck

This comment was marked as resolved.

Sign in to view
jmdyck
jmdyck requested changes Aug 20, 2024
View reviewed changes
spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
@michaelficarra michaelficarra force-pushed the iterator-helpers-integration branch 2 times, most recently from 2255ce1 to 0cb2376 Compare September 5, 2024 00:03
@michaelficarra
Copy link
Member Author

FYI we're having a relevant editorial discussion here: tc39/proposal-joint-iteration#30

michaelficarra and others added 14 commits September 23, 2024 15:10
@michaelficarra
Normative: add Iterator Helpers
1f510fe
@michaelficarra
rename enums in GetIteratorFlattenable
d411a53 
also split up the steps and add an assert for better understandability
@jmdyck @michaelficarra
reorder the Iterator.* and Iterator.prototype.* sections
b6e155e 
The spec's convention is:
when you have a set of sections that define the properties of some intrinsic object,
first do the properties whose keys are strings, in alphabetical order,
then do the properties whose keys are symbols, in alphabetical order.

(The only exception is in "Properties of the RegExp Prototype Object",
where the symbol-keyed-properties are interleaved with the string-keyed-properties.)
@jmdyck @michaelficarra
Convert @@ notation to intrinsics notation (PR #1314)
a40b31a
@jmdyck @michaelficarra
Use better (more consistent) ids
a3c1221
@jmdyck @michaelficarra
Declare %IteratorHelperPrototype% and %WrapForValidIteratorPrototype%
b068cd5 
... as well known intrinsics.

(You'll need to fill in the descriptions.)
@jmdyck @michaelficarra
Wording tweaks
e0ef2e8
@jmdyck @michaelficarra
Add a <p><ul> to "Properties of the Iterator Constructor"
18b5931 
All other "Properties of the Foo Constructor" sections
start with a <p><ul> where, at the very least,
you say what the constructor's [[Prototype]] is.
@jmdyck @michaelficarra
%WrapForValidIteratorPrototype% is an ordinary object?
9a54fa6
@michaelficarra
change parameter name for forEach
a8ad965
@michaelficarra
accept jmdyck's suggestions
5b3db4a
@michaelficarra
return ~unused~ from closures passed to GeneratorStart
48fd369
@michaelficarra
the AC passed to CreateIteratorFromClosure should return completions
d4c0e3c
@michaelficarra
ReturnCompletion(*undefined*) instead of NormalCompletion(~unused~)
380047c
ljharb
ljharb reviewed Sep 26, 2024
View reviewed changes
spec.html Show resolved Hide resolved
ljharb
ljharb reviewed Sep 26, 2024
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR seems to be missing changes to CreateIteratorFromClosure to add a 4th argument? (present in the proposal spec)

syg
syg approved these changes Sep 26, 2024
View reviewed changes
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
@michaelficarra michaelficarra mentioned this pull request Sep 27, 2024
Closed
@michaelficarra michaelficarra added has stage 4 This PR represents a proposal that has achieved stage 4, and is ready to merge. and removed pending stage 4 This proposal has not yet achieved stage 4, but may otherwise be ready to merge. labels Oct 8, 2024
@fisker fisker mentioned this pull request Oct 9, 2024
Open
@michaelficarra
Copy link
Member Author

@tc39/ecma262-editors Do we want to make the [[UnderlyingIterator]] internal slot plural (as is required by upcoming proposal joint iteration) as part of this PR, or should we do it in 2 phases?

@michaelficarra michaelficarra added the editor call to be discussed in the next editor call label Oct 10, 2024
@azu azu mentioned this pull request Oct 10, 2024
Open
bakkot
bakkot approved these changes Oct 17, 2024
View reviewed changes
Copy link
Contributor

@bakkot bakkot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I no longer think we should rename [[UnderlyingIterators]] now, since it would also involve changing its handling; we can do that in the followup.

@michaelficarra michaelficarra added ready to merge Editors believe this PR needs no further reviews, and is ready to land. and removed editor call to be discussed in the next editor call labels Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb left review comments

@jmdyck jmdyck jmdyck requested changes

@syg syg syg approved these changes

@bakkot bakkot bakkot approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
has stage 4 This PR represents a proposal that has achieved stage 4, and is ready to merge. normative change Affects behavior required to correctly evaluate some ECMAScript source text proposal This is related to a specific proposal, and will be closed/merged when the proposal reaches stage 4. ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@michaelficarra @jmdyck @ljharb @syg @bakkot