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 parsing through enums/structs instead of vecs/strings #158
base: main
Are you sure you want to change the base?
Conversation
…djust rest of workspace to work with it
I've ran this with some of my own code and it looks like the parsing works correctly now (though I'd still like to write some more tests for it), but the diffing has issues (it panicked upon a code update being ran with glicol-cli) so I need to fix that. |
Ok, I think I may have fixed the panicking thing (i forgot a single |
I just tested the WASM part a little bit: when there is an syntax error, we get a panic and the sound stops. Ideally, the music should continue and only report the error. |
Would you be able to share how you're testing it in wasm, or what the stack trace is? I've only been trying this with glicol-cli so far, so I haven't had a chance to try it out with the wasm module, though I did try to make sure it still worked there. |
First run https://github.com/chaosprint/glicol/tree/new-parser-js-test Diffs can be found here: 351605a Next, in the ./js folder, run In the browser (preferably Chrome) we can see a simple editor for testing and all the error message from the browser console. Let me know if you have any questions. But in general, I think it's fine to leave the wasm/js part broken and solve it later since this is a breaking change targeting 0.14 anyway. I will test glicol-cli as well tmr. |
Ok, I finally tried out the js build and it seems to work fine on my machine - the pretty-print errors even appear in the console now, which seems cool. As one side note - it might be smart to run Also, would you (@chaosprint) be able to look at the |
... and adjust rest of workspace to work with it.
Closes #134.
Sorry that this is all one commit, I couldn't think of a way to split it up since basically every part of this depends on another part.
The big change comes in the
rs/parser/src/nodes.rs
file - this implements all the various structs and enums that exist in the grammar, as best as I could determine. When I was converting the main crate over to parsing this AST, I ran into a few situations where the grammar inglicol.pest
and the way that it was turned intoglicol_synth
nodes seemed to disagree, and I just had to make my best judgement in those situations. Sometimes I modified the struct in the AST, and sometimes I just added apanic!
with an explicit error message about how that feature/syntax isn't supported yet. If thepanic
message is inaccurate and that feature won't ever be supported, let me know and I'll modify the structs to fail at parsing that sort of thing.The second biggest changes are in
rs/main/src/{lib,util}.rs
. Two things happen here:util.rs
, the AST is converted into nodes to be placed on the graph. This was fairly straightforward, and allowed us to remove a ton ofunimplemented!
calls (since all enums used are now exhaustive and have no invalid states, instead of just usingGlicolPara
as a catch-all).lib.rs
, I changed the way we update the engine with code. Previously, we would update the code, then set the flagneeds_update
, then actually diff and update the graph the next timenext_block
was called. I felt like this was not ideal, as it causes the AST and code to be out-of-sync. So instead I switched it to makeupdate_with_code
actually do the diffing and updating of the syntax tree, andnext_block
just gets the next block of sound.This required some undesirable combination of methods into one large method that does a bunch of stuff, but I think that it's for the best. It allows us to remove a bunch of members from the
Engine
struct that are only there temporarily for diffing, and put those into another struct that borrows fromself
and the newYokedAst
for diffing, instead of copying data over to the temporary members ofself
during the diffing process.I also got rid of the
lcs_diff::diff
method and elected to do diffing manually, as that method required cloning of each AST struct that was passed in, and that's an unnecessary cost to pay. Instead, we just iterate through each new and old node + chain and see what stayed the same, what was removed, and what was added.A few other minor things:
GlicolPara
is now generic overS
so that we can use it in situations where we want to borrow from something else to create like aGlicolPara::Reference
to avoid another allocationEngineError
to a&mut [u8]
is now moved to thewasm
crate, since that's the only thing that actually needs that code.wasm
crate is now part of the workspace since it needs to accesspest
(to match details of theEngineError
, so its profile details have been moved to a profile calledwasm-release
(instead of just using therelease
profile, as was previously done)pest
errors are now boxed in our return types since they are quite large and thus make everyResult
type that contains them quite large as well, incurring a significant cost on method returns even if we don't encounter an error. Now, they shouldn't be quite as large, at the cost of only one allocation if we run into an error (as opposed to incurring costs before regardless of if we ran into an error).The main issue with this all is that I haven't actually tested it yet. Before merging, I'd like to get some more formal tests written inside the parser crate, and I'd also like to just try this out with something like
glicol-cli
to make sure everything seems to still work just fine.On that note, this should also allow much clearer error messages in
glicol-cli
that can point to exactly where we failed to parse. This should make for a much better experience there, once this gets merged and I can push a PR toglicol-cli
to work with this new API.Of course, this will also change the public API, so all these crates should be bumped to
0.14.0
if this gets merged.If you have any questions, obviously let me know! And please don't merge until I've got some tests in (just as some motivation for myself).