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

Implement Iterator.prototype.drop #673

Merged
merged 1 commit into from
Nov 11, 2024
Merged

Implement Iterator.prototype.drop #673

merged 1 commit into from
Nov 11, 2024

Conversation

saghul
Copy link
Contributor

@saghul saghul commented Nov 11, 2024

Includes the scaffold for other iterator helper methods that require an Iterator Helper object.

Copy link
Contributor Author

@saghul saghul left a comment

Choose a reason for hiding this comment

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

Not 100% done yet, but reviewable!

Edit: ready for review now.

quickjs.c Show resolved Hide resolved
quickjs.c Outdated Show resolved Hide resolved
quickjs.c Show resolved Hide resolved
@saghul saghul marked this pull request as ready for review November 11, 2024 12:57
@saghul
Copy link
Contributor Author

saghul commented Nov 11, 2024

@bnoordhuis PTAL, Tests pass locally, this should be good for review!

Once this one lands I expect the rest to be simple to add, I scratched my head a few times while working on this, but I feel like I got a much better understanding as to how iterators work internally!

@saghul saghul requested a review from bnoordhuis November 11, 2024 12:59
@saghul
Copy link
Contributor Author

saghul commented Nov 11, 2024

Hum I cannot spot the leak with the naked eye, I'll take a closer look tonight, together with the feedback I'll surely get :-)

Green!

abort();
}

done:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if we should free the wrapped iterator after we are done with it?

Copy link
Contributor

Choose a reason for hiding this comment

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

At terminal state, when the helper won't touch the wrapped iterator anymore? I would assume so.

Probably doesn't matter from a correctness perspective (not observable), only for efficiency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I'll leave this for the end because I suspect some simplification will happen once all remaining methods are in.

quickjs.c Outdated Show resolved Hide resolved
quickjs.c Outdated Show resolved Hide resolved
abort();
}

done:
Copy link
Contributor

Choose a reason for hiding this comment

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

At terminal state, when the helper won't touch the wrapped iterator anymore? I would assume so.

Probably doesn't matter from a correctness perspective (not observable), only for efficiency.

Includes the scaffold for other iterator helper methods that require an
Iterator Helper object.
@saghul saghul force-pushed the iterator-helpers-drop branch from 6fdc48a to 92cd712 Compare November 11, 2024 21:14
@saghul saghul merged commit c41ee4f into master Nov 11, 2024
47 checks passed
@saghul saghul deleted the iterator-helpers-drop branch November 11, 2024 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants