Runelookup to avoid testing regexps that have no chance to match #111
Conversation
There was a problem hiding this comment.
Let's merge this in, but please see the comment re. FIXME's and fold that into the next PR. The only FIXME's that should remain in production code are for potential improvements, but never bugs or edge cases that users will run into. The rationale here is that in reality FIXME's never get fixed.
| // - what possible first runes it would match on | ||
| // - what possible match size can be expected (possibly zero, exactly one, one or more) | ||
| // The possible match size is for now not utilized, but could potentially be used to not | ||
| // even launch a regexp match if the rune is found is in the input. |
| // since otherwise it would take too much memory. | ||
| const ( | ||
| charclassSizeLimit = 4096 | ||
| mayMatchZero = 0 |
There was a problem hiding this comment.
BTW for future reference this incrementing constants is solved by iota:
const (
mayMatchZero = iota
mayMatchOneOrMore
matchesExactlyOne
)(yes, the syntax is bizarre)
| return []*syntax.Regexp{r}, matchesExactlyOne | ||
| } | ||
|
|
||
| panic("should not get here") |
There was a problem hiding this comment.
I would typically also include contextual information in the panic, otherwise it can be very difficult to debug in "real life".
| "regexp/syntax" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" |
There was a problem hiding this comment.
FYI, somewhat counter-intuitively, assert.* functions do not fail the test immediately, they continue on. This is almost never what you want, but this behaviour is available in testify/require.
|
|
||
| var rule = &rules[i] | ||
|
|
||
| // FIXME: what about one char rules ? |
There was a problem hiding this comment.
Do these FIXME's matter? ie. will user's encounter them or not?
If they will, they should be fixed now, as (generally) FIXME's are never fixed. Ironic.
| } | ||
| return lexer.Token{}, err // FIXME |
There was a problem hiding this comment.
Same as above, plus this has no context. Fix what? Delete the FIXME if it's not a real issue.
…tch (#111)" This reverts commit 7621a32. -- I've reverted this for a couple of reasons. The first is just a bug that could be fixed, in that ordering of rules with the same prefix aren't respected (eg. `${` vs. `$(`). The second reason is that on reflection I think that Participle will be better served by keeping the default dynamic implementation fairly simple and investing most optimisations into code generation. To that end, while this lookup does bring some improvement (~5%-10% from memory), it is also quite complex.
…tch (#111)" This reverts commit 7621a32. -- I've reverted this for a couple of reasons. The first is just a bug that could be fixed, in that ordering of rules with the same prefix aren't respected (eg. `${` vs. `$(`). The second reason is that on reflection I think that Participle will be better served by keeping the default dynamic implementation fairly simple and investing most optimisations into code generation. To that end, while this lookup does bring some improvement (~5%-10% from memory), it is also quite complex.
…tch (#111)" This reverts commit 7621a32. -- I've reverted this for a couple of reasons. The first is just a bug that could be fixed, in that ordering of rules with the same prefix aren't respected (eg. `${` vs. `$(`). The second reason is that on reflection I think that Participle will be better served by keeping the default dynamic implementation fairly simple and investing most optimisations into code generation. To that end, while this lookup does bring some improvement (~5%-10% from memory), it is also quite complex.
Let's do it.