-
-
Notifications
You must be signed in to change notification settings - Fork 38
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
Draft: some rewrite thing #115
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had a quick scan through and will do a more thorough review in the week - but to start it looks like a lot of the existing tests have been changed or commented out or just deleted - is this just while rewriting or are you planning on these changes being permanent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love better error reporting in Leaf, so I'm really supportive of any efforts to achieve this @pontaoski ❤️ Thanks for your explorations!
That being said, I've had a look through the proposed changes, and it's a large amount to change in one go. There could be an amazing set of improvements in here, but it's putting the onus on the reviewer to wade through thousands of lines of code and unstructured commits to work that out.
Are you willing to consider breaking the PR into a series of smaller changes and improvements? Perhaps there is a logical sequence of individual improvements that could be separated out and proposed as individual PRs in a chain?
I'm happy to help do this if you'd like.
4d7efb9
to
18147c8
Compare
in my experience regarding error reporting in parsers, you kinda need the capabilities to be worked in from the start of the design, otherwise redoing the whole thing from the ground up is much easier to understand, since you only need to validate that a brand new lexer+parser is doing the right thing is way easier than trying to validate that you've correctly hacked on better error reporting to an existing design. additionally, unfortunately with the way leaf is designed, there was an extremely tight coupling between the data structures involved in the lexer+parser+renderer+evaluator, so any change of lexer+parser necessitates a lot of changes in the latter. that being said, i've adjusted the file structure to make the git diff render all of the rewrites as new files instead of attempting to diff with the files they replace, which should help with legibility. |
18147c8
to
ceece91
Compare
That makes sense. I also appreciate that you filed this PR so that these conversations could happen around how best to integrate your changes, so thank you for being open to that. To get this PR to a point where it can be properly reviewed and merged, I think there are a few things that need to happen:
Sorry for the slow replies today, it's been a long work day. |
075d59b
to
fb7cac2
Compare
This rewrites the lexer, parser, and evaluator to have *much* simpler code, as well as better error reporting and other things generally considered nice in modern programming language implementations.
fb7cac2
to
205d067
Compare
… specify the variable name of index for arrays) Fixes vapor#105.
poke |
This PR is some sort of mess of me deciding to make the scanner have better errors, then ending up rewriting the scanner, then ending up rewriting the parser that consumes that scanner, then ending up rewriting the evaluators that consume ASTs from the parser.