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

feat: Add iterator methods to Query #207

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

segevfiner
Copy link
Contributor

@segevfiner segevfiner commented Jul 9, 2024

Add iterator methods to Query

Still need to do capturesIter and CapturesIterator, typings, etc.

I'm not that happy about the code duplication though... We could probably implement matches and captures in terms of matchesIter and capturesIter to reduce it. But that might be slower due to all the additional calls that would entail between C++ and JS, and transferring the nodes one by one.

P.S. LookaheadIterator[@@iterator] can probably be implemented in C++, and ts_query_cursor can probably be allocated on Query instead of in AddonData if we want too.

Fixes #178

@segevfiner segevfiner changed the title WIP: feat: Add iterator methods to Query feat: Add iterator methods to Query Jul 10, 2024
@segevfiner segevfiner marked this pull request as ready for review July 10, 2024 18:52
@amaanq
Copy link
Member

amaanq commented Aug 17, 2024

I think it'd make more sense to just implement matches and captures with IterableIterator by default, what do you think?

@segevfiner
Copy link
Contributor Author

I'm worried about backwards compat abd performance. Which is why I kept the existing ones for now.

@amaanq
Copy link
Member

amaanq commented Aug 17, 2024

If performance is a concern, why even add this then? What's the benefit/purpose exactly? (really curious because I'm all for iterators)

@segevfiner
Copy link
Contributor Author

segevfiner commented Aug 17, 2024

Memory consumption. As using iterators means we don't need to hold all results in memory at once. We still need to measure if it's really significantly slower or not. I'm not sure if it's really slower. But if it is, it is always possible to chunk the iteration internally so we don't transfer one by one.

@amaanq amaanq force-pushed the master branch 3 times, most recently from 4e43908 to ff76451 Compare November 10, 2024 22:38
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.

Consider making Query matches/captures iterators
2 participants