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

Pattern Matching #76

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Pattern Matching #76

wants to merge 15 commits into from

Conversation

AlexMax
Copy link
Contributor

@AlexMax AlexMax commented Feb 10, 2017

So yeah, pattern matching. Hooray.

  • All of the "pm.lua" tests pass.
  • This implementation sticks pretty close to the original C implementation - I figured it was better to be safe than sorry. Comments from the original implementation have been preserved to better orient code spelunkers.
  • Passing pointers to string positions isn't really a thing in Go, and I wasn't sure I could replicate some of the pointer arithmetic of the original code using slices, so instead I pass around string positions as integers.
  • Several instances of relying on returning a NULL pointer as a failure state have been converted into Go-style multiple-return with an ok boolean.
  • The original implementation uses goto in the main match function in order to keep the stack under control, and this has been preserved with slight alterations in order to accommodate Go's stricter rules on where you can and can't jump to. In my research, it appears that Go stacks are (nearly) infinite, but I'm not sure that means converting the jumps to recursive function calls is necessarily a good idea..

Also, my apologies, but I actually started this branch midway through #67, so I would merge that before looking at this. I also added a little helpful bit to the Lua test harness that returns the error string off the stack if one exists when running the test suite - if you would rather that be in a separate pull request, it can be backed out.

- Error messages did not properly chunkID the source filename, leading
to @'s next to filenames and such.
- LoadFile improperly raised a generic file read error, even if the
actual error was a Lua error such as a SyntaxError.
Currently implements only a very limited subset of `gmatch`.

Yes, there is a goto in this commit - it was in the original codebase.
In the original C version of match, there is a label after the default
case that can be jumped into from above.  The goto implementation in Go
does not allow us to jump to that label in all cases.  So instead, we
form a closure from the default case that we can call into anyplace.
Enabling pattern-matching tests also opened the door to fixing a couple
of parsing bugs and filling out a few additional features, such as +, *,
and $.
- When looking for the end of a square-bracket class, properly skip past
an escaped character.
- Properly check for all forms of punctuation in %p test, not just what
unicode considers punctuation.
- Fix out-of-bounds read in gsub function.
- Implement balanced string and frontier matching.
- Fix error messages to actually show up.
- Fix a few nasty out of bounds slice accesses.
- Run gmatch against the very last position of the source string.
The original version of match() used goto labels to avoid recursive
function calls and keep control over the stack size.  However, Go cannot
jump from one block to another, so the original location of the "dflt"
label was unusable.  Until this commit, I worked around this deficiency
with a closure, but this made the code harder to follow and
"out-of-order" compared to the original codebase.

The closure is gone, and instead we move the default case of the main
switch out into the parent block, reintroducing the old "dflt" label.
In cases where we do not want to execute the "dflt" label, we skip over
it with a new label that goes straight to the "end" of the function.
@jorgevillafuerte
Copy link

Hello, Any update with this PR? Why is not merged? Thanks

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

2 participants