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

More tweaks for the iterator handling AOs #3311

Merged
merged 1 commit into from May 22, 2024
Merged

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Apr 5, 2024

The iterator protocol has a bunch of helper AOs. But almost everything just uses IteratorStepValue (introduced in #3268) and IteratorClose; the exceptions are

This PR changes IteratorStep and IteratorNext so that they set the [[Done]] slot on the Iterator Record to true when some part of the protocol throws or the iterator result object's .done is found to be true, consistent with how IteratorStepValue behaves. (I'd previously considered eliminating IteratorStep entirely, but "I need to advance this iterator but I don't care about the value" is a coherent thing and does come up sometimes.)

Of the above uses, only the "destructurings with elisions" case actually care whether the [[Done]] slot gets set to true, which it was previously doing explicitly. Still, it seems nicer to handle these uniformly, such that manipulation of that slot is done only in the iterator AOs themselves.

This also incidentally eliminates 2 of the remaining 4 explicit uses of the ReturnIfAbrupt macro (cf #1573 (comment)), the last two being in [[Call]] and [[Construct]] for ECMAScript function objects.

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.

I like it.

Drive-by: looking at the description for [[Done]], I don't really like "Whether the iterator has been closed". It almost reads to me as whether IteratorClose has been called on it. I don't have a better suggestion at the moment though, I think I just narrowly don't like the word "closed" here.

@bakkot
Copy link
Contributor Author

bakkot commented Apr 17, 2024

Maybe "Whether the iterator has completed or been closed"?

@syg
Copy link
Contributor

syg commented Apr 17, 2024

Maybe "Whether the iterator has completed or been closed"?

sgtm!

@bakkot bakkot added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label May 21, 2024
@ljharb ljharb merged commit c4c55b6 into main May 22, 2024
8 checks passed
@ljharb ljharb deleted the iterator-step-closes branch May 22, 2024 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

4 participants