Pass operator to Pratt parsing functions #514
Conversation
|
@Zij-IT Any chance you'd have a free moment to take a look at this? |
I’ll give it a look this weekend (: Edit: For some reason I thought it was Friday 🤦♂️ I'll give it a look in the next couple days |
There was a problem hiding this comment.
Overall I love the work, and I think you did a great job. @zesterer has a similar PR that also moves us closer to the API using Fn as opposed to the current API which uses fn, and also allows us to use multiple combinations of functions, such as the one you would prefer, and also one that has a total span.
I'm curious what you would think of his variation, and if you feel there are any items missing in that implementation? If you haven't seen it, you can find it here #515
| @@ -15,7 +15,7 @@ exclude = [ | |||
| build = "build.rs" | |||
|
|
|||
| [features] | |||
| default = ["std", "spill-stack"] | |||
There was a problem hiding this comment.
I believe this gets fixed in a following commit, but tagging just in case :)
| where | ||
| I: Input<'a>, | ||
| E: ParserExtra<'a, I>, | ||
| InfixOps: Parser<'a, I, InfixOpsOut, E>, |
There was a problem hiding this comment.
Any particular reason this trait bound has to disappear? It was originally added with the idea that it makes no sense to construct a Parser which cannot parse.
There was a problem hiding this comment.
It doesn't have to disappear, but it does have to change a bit. At first I didn't figure out how to change it, so I haphazardly deleted it.
| >, | ||
| > | ||
| where | ||
| I: Input<'a>, | ||
| E: ParserExtra<'a, I>, | ||
| PrefixOps: Parser<'a, I, PrefixOpsOut, E>, |
There was a problem hiding this comment.
Any particular reason this trait bound has to disappear? It was originally added with the idea that it makes no sense to construct a Parser which cannot parse.
| type InfixBuilder<E> = fn(lhs: E, rhs: E) -> E; | ||
| type InfixBuilder<Op, O> = fn(op: Op, children: [O; 2]) -> O; |
There was a problem hiding this comment.
I'm not particularly a fan of this, although I understand why it has to happen. That aside, E I think should have become Expr if a name change was necessary.
| type PostfixBuilder<E> = fn(rhs: E) -> E; | ||
| type PostfixBuilder<Op, O> = fn(op: Op, child: O) -> O; |
There was a problem hiding this comment.
This is one of the points that I was unsure about. I think I would prefer and find it more natural for PostfixBuilder to be fn(O, Op) -> O because the op follows the expression when parsing.
|
Oh sweet, zesterer might have built an even more general version. I'll check it out and am very happy with closing this in favour of a better solution! As long as I end up being able to use an excellent parser combinator library, I'm very satisfied. |
|
Deprecated in favor of #515 |
Deprecated in favor of #515
Fix #507
I'm not sure if this implementation is ideal, so I'll appreciate any and all feedback.
Things that could definitely still be done
Pinging @Zij-IT : I'd love to get some feedback on this.