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: Introduce AO combining IteratorStep and IteratorValue #3268

Merged
merged 1 commit into from
Jan 31, 2024

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Jan 25, 2024

"IteratorStepValue" isn't a great name, but I'm worried that sticking with IteratorStep will lead to a bunch of confusing documentation/comments, so I didn't want to reuse it.

In the cases which deal with completion records explicitly you might have to think for a second to convince yourself the change doesn't actually affect behavior.

The only 2 remaining uses of IteratorStep are in the elisions in [a, /* hole */, b] = iterable. Ordinarily I'd inline it into those and delete the AO, but I want to update 402 and proposals to use the new AO before doing that.

There are still 8 uses of IteratorValue: 7 in yield*, 1 in the AsyncFromSyncIterator wrapper. yield* could probably use a cleanup too, but not in this PR.

spec.html Outdated Show resolved Hide resolved
spec.html Outdated
Comment on lines 19430 to 19431
1. If _next_ is ~done~, return _list_.
1. Append _next_ to _list_.
Copy link
Member

Choose a reason for hiding this comment

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

should all of these have an assertion that next is an ECMAScript language value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eh, I don't think it's necessary. It's not hard to tell given the type of IteratorStepValue.

@bakkot bakkot force-pushed the iterator-step-value branch from d4a4230 to be7ec12 Compare January 25, 2024 07:01
Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

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

This is great.

spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
@ljharb ljharb requested a review from a team January 26, 2024 16:16
jmdyck
jmdyck previously requested changes Jan 26, 2024
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
@bakkot
Copy link
Contributor Author

bakkot commented Jan 26, 2024

@jmdyck done, thanks (though in a slightly different form, following #3273).

@jmdyck
Copy link
Collaborator

jmdyck commented Jan 26, 2024

Okay. That form isn't defined in 5.2.3.4, but the meaning is pretty clear. (Throw it on the pile for PR #1573.)

@bakkot
Copy link
Contributor Author

bakkot commented Jan 27, 2024

Fun fact: this eliminates 24 of the 28 explicit uses of the ReturnIfAbrupt macro. The remaining 4 are: one in [[Call]] for ES functions, one for [[Construct]] for ES functions, and two in the handling of [a, /* hole */, b] = iterable elisions (almost immediately after each of the two remaining uses of IteratorStep mentioned above).

It's tempting to remove it entirely, and replace #1573 with a direct definition of ? instead of defining it in terms of ReturnIfAbrupt.

1. Return _groups_.
1. Let _value_ be ? IteratorValue(_next_).
1. Let _value_ be _next_.
Copy link
Member

Choose a reason for hiding this comment

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

I would've just never introduced next, but this is fine.

@bakkot bakkot added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Jan 30, 2024
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.

5 participants