Conversation
jhugman
left a comment
jhugman
left a comment
There was a problem hiding this comment.
@ip1981 Thank you so much for fixing this!
It's been on my list for so long, but I haven't had the brain space to learn how lalrpop works.
I've added a couple of comments below: the biggest thing to fix is the precedence ordering, and naming.
I think you'll likely end up making an UnOp60 (or similarly named rule).
Once you get operator precedence squared away, re-request a review from me!
Many thanks, again!
jexl-parser/src/parser.lalrpop
Outdated
| "^" => OpCode::Exponent, | ||
| }; | ||
|
|
||
| Op60: UnOpCode = { |
There was a problem hiding this comment.
Nit: given this is at Expr70 level, to follow the rest of the code, could we name this Op80?
jexl-parser/src/parser.lalrpop
Outdated
| Expr70: Box<Expression> = { | ||
| <subject: Expr70> <index: Index> => Box::new(Expression::IndexOperation{subject, index}), | ||
| <subject: Expr70> "." <ident: Identifier> => Box::new(Expression::DotOperation{subject, ident}), | ||
| <operation: Op60> <right: Expr80> => Box::new(Expression::UnaryOperation { operation, right }), |
There was a problem hiding this comment.
Should . and [ bind tighter than !? Playground with the jexl-js.
I couldn't see a test, and I'm not familiar enough with lalrpop know. Please could you add tests verifying the following work:
foo == [false]
!foo[0] == true
foo == { "bar": false }
!foo.bar == true
There was a problem hiding this comment.
Ok, I've updated my patch fixing priorities, naming, adding the tests you suggested.
|
I would be great if this PR could be merge to implement the |
...and also it seems it would help to make the unary minus to work (issue #27)! |
Signed-off-by: Igor Pashev <pashev.igor@gmail.com>
|
Ping |
4 participants
No description provided.