-
Notifications
You must be signed in to change notification settings - Fork 28
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: Use nom-locate to add span information in the AST #129
Conversation
Thanks! I’ll try to dedicate some time to read this asap! |
In the last commit I decided to try implementing a This requires quite a lot of changes since this introduces lifetimes everywhere, and the Using a parsing context will allow replacing the |
glsl/src/parse_tests.rs
Outdated
|
||
let expected = syntax::TranslationUnit(syntax::NonEmpty(vec![block])); | ||
|
||
assert_eq!(translation_unit(src), Ok(("", expected))); | ||
assert_eq_parser(translation_unit, src, Ok(("", expected)), &ctx); |
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.
Currently broken on Windows because of how new lines are checked out by git with autocrlf on
Spans are now owned by nodes instead of the parsing context. This simplifies a big part of the code (and probably more when I finish refactoring). I think it's a better idea to have span information working first, and then to optimize how this is handled if it proves necessary. To avoid having too many changes required on the parsing tests, there is now a |
With the proc-macro in place we can also automatically generate type aliases for |
The main target for this PR is issue #74. By using nom_locate we can rewrite the parsing functions to operate on input with span information added in. I'll post an update when the functionality is usable (for now I've just changed the parser and tests to use
nom_locate::LocatedSpan
).This is flagged as WIP to discuss how to integrate this in the current architecture. One potential side-effect is allowing to parse comments (as requested in #116). We could have a way to associate "non-significant" spans (whitespace and comments) to the relevant syntax nodes in order to parse comments relative to the syntax tree, as suggested in https://www.oilshell.org/blog/2017/02/11.html.
This one is more of a stretch and it might be more sensible to be a second iteration on this feature, but this would allow preserving pre-processing directives in non-top-level contexts in the same way, which is currently broken (see issues #117 and #64).