-
Notifications
You must be signed in to change notification settings - Fork 214
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
add a new (split) PEG special #1346
Conversation
With respect to |
For any other folks who might be checking this PR out, highly recommended are the tests for gaining some familiarity. As the tests demonstrate, it seems worth noting that captures get dropped from the separator pattern and that the separator pattern doesn't have to match anything. That the "separator can be an arbitrary PEG" is especially nice (possibly one of the main motivators?) and what inclines me toward a |
ee5eee9
to
8dddb0c
Compare
I went ahead and added a |
Working well here so far. Nice that the |
So I've been studying (peg/match ~(split "," (capture (to -1)))
"a,,bar,c") when What I'm seeing in the locals view is this:
The rest of the locals view at the time looks like this:
IIUC, The code works fine here but this feels odd. May be it's nothing to worry about? [1] See this gist for using |
Hey thanks! That's interesting. It does seem wrong to advance past the input; I don't know if anything bad would happen but it seems plausible. I don't think any other rule does it, so I just pushed a fix although I'm not really sure how to write an automated test for this; I'll try harder when I get off work. |
I've no clue how one would write a test for this. Perhaps it isn't necessary to do so. |
This works similarly to string/split, but the separator is a PEG.
I found a test that failed with the old behavior but not the new behavior: the rule |
Nice that you found that test and thanks for the fix 🎉 |
So I don't like how |
the TO_END rule also looks good and seems to be a good way to possibly speed up some matches. That said, I think for more optimizations a benchmark suite is needed. I'm reluctant to add performance optimizations without measurement unless they are trivial. |
Oh yeah, I didn't think of that! It would be possible but maybe crazy to add a I just removed that commit for now; I'm not really convinced the function is pulling its weight if it requires re-implementing the split logic. |
Ok, currently LGTM. Thanks @ianthehenry ! |
This works similarly to
string/split
, but the separator is a PEG.I also added a
peg/split
helper function, and a new opcode to optimize the(to -1)
pattern so that it will only do two passes over the input instead of three.