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.

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

Editorial: Add Iterator Record type #2591

Merged
merged 1 commit into from
Feb 16, 2022

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Nov 28, 2021

This adds a <dfn> and <emu‑table> for “Iterator Records”.


Part of #2486

spec.html Outdated Show resolved Hide resolved
@ExE-Boss ExE-Boss force-pushed the editorial/add-iterator-record-type branch from e1987d5 to 623951d Compare November 29, 2021 07:38
@ExE-Boss ExE-Boss requested a review from jmdyck November 29, 2021 07:39
Copy link
Collaborator

@jmdyck jmdyck left a comment

Choose a reason for hiding this comment

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

In 27.1.4.3 "Properties of Async-from-Sync Iterator Instances", the description for [[SyncIteratorRecord]] says

A Record, of the type returned by GetIterator, representing ...

That could be changed to

An Iterator Record representing ...

Other than that, looks good to me.

@ExE-Boss ExE-Boss requested a review from jmdyck December 10, 2021 20:22
@michaelficarra
Copy link
Member

Partially resolves #2486.

Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

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

LGTM

@michaelficarra michaelficarra requested a review from a team December 20, 2021 20:13
spec.html Outdated Show resolved Hide resolved
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 other than comment.

jmdyck
jmdyck previously requested changes Dec 29, 2021
Copy link
Collaborator

@jmdyck jmdyck left a comment

Choose a reason for hiding this comment

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

I just noticed that you could also change the 'type' of the _iteratorRecord_ parameter from unknown to an Iterator Record in each of:

  • IteratorBindingInitialization
  • IteratorDestructuringAssignmentEvaluation
  • ForIn/OfBodyEvaluation
  • PerformPromiseAll
  • PerformPromiseAllSettled
  • PerformPromiseAny
  • PerformPromiseRace

And similarly for _syncIteratorRecord_ in CreateAsyncFromSyncIterator.

@michaelficarra michaelficarra added the editor call to be discussed in the next editor call label Feb 15, 2022
@bakkot bakkot added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Feb 16, 2022
Co-authored-by: ExE Boss <[email protected]>
Co-authored-by: Michael Dyck <[email protected]>
@ljharb ljharb force-pushed the editorial/add-iterator-record-type branch from 2a943e6 to 85d910c Compare February 16, 2022 23:14
@ljharb ljharb dismissed jmdyck’s stale review February 16, 2022 23:15

per editor call, this can be done in a followup

@ljharb ljharb merged commit 85d910c into tc39:main Feb 16, 2022
@michaelficarra michaelficarra removed the editor call to be discussed in the next editor call label Feb 16, 2022
@ExE-Boss ExE-Boss deleted the editorial/add-iterator-record-type branch February 17, 2022 11:40
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Apr 18, 2022
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Apr 21, 2022
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Apr 22, 2022
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Apr 28, 2022
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Aug 6, 2022
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Aug 16, 2022
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Aug 30, 2022
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Sep 3, 2022
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial change ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants