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

[WIP] [zero-copy] Static analysis #295

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

CraftSpider
Copy link
Collaborator

This is all slightly cursed, and horribly WIP, but I wanted it up for viewing

@zesterer
Copy link
Owner

This is neat! I like the trait-based API. I feel like this is worth putting behind a feature flag though.

@CraftSpider
Copy link
Collaborator Author

Yeah, probably. I'll add the feature flag, and try to flesh out more of the parsers with implementations. I'm still not super happy with how much state the visitor has to carry around, but I'm not sure how to do it much better while still being generic over any possible parser.

@m-hugo
Copy link

m-hugo commented Mar 14, 2023

Why is zesterer:zero-copy still targeted instead of zesterer:main ? it makes the diff unreadable

@CraftSpider CraftSpider changed the base branch from zero-copy to main March 14, 2023 02:12
@CraftSpider
Copy link
Collaborator Author

By mistake - fixed the target

@zesterer
Copy link
Owner

Is this ready for review?

@CraftSpider
Copy link
Collaborator Author

I'm iffy on this one still - I'm not super happy with the API, but if you want to get it in and not expose it till later, I think it's at least ready for that.

@m-hugo
Copy link

m-hugo commented Mar 17, 2023

why not expose this and pratt in an experimental (use chumsky::experimental::{problems, pratt}) module ?
that way it's not in indefinite limbo and you can rework it later

@zesterer
Copy link
Owner

Adding this later wouldn't be a breaking change, so I don't think there's any particular pressure to push this (or the pratt parser) right now. If @CraftSpider feels like it's ready, then great: but I think I'd generally err on the side of waiting a little bit or releasing it in a future alpha (1.1, perhaps) to get a feel for what the API should be able to do.

@CraftSpider
Copy link
Collaborator Author

Once I have the motivation and time, I'll prioritise improving the API here, to be clear. It's just in limbo right now because I've both been busy and struggling to draw up the motivation to work on it.

@zesterer
Copy link
Owner

Once I have the motivation and time, I'll prioritise improving the API here, to be clear. It's just in limbo right now because I've both been busy and struggling to draw up the motivation to work on it.

That's ok! I find my motivation comes and goes a lot too. It definitely shows in my commit history :D

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.

None yet

3 participants