-
-
Notifications
You must be signed in to change notification settings - Fork 115
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 support for bitwise operators #425
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes, I checked out your branch to see what error you got and it's definitely one of our least helpful errors. That error just means that there's an ambiguity in the grammar.
In this case, it's because ^
is both an infix and a prefix operator (I knew it would be ^
that bit us!). The parser can't tell which you mean whenever you write ^
. This also made me realize that unboxing is currently a function bound to the identifier ^
. To actually get both to work, we'd need to make it something statement-like which would be a bit out of scope for this PR (and would also be a breaking change), so I think the move is to just leave ^
out for now.
Also, it looks like you just didn't add the new tokens to the lexer:
https://github.com/grain-lang/grain/blob/master/compiler/src/parsing/lexer.mll#L194-L196
Forgetting that one would just give you a regular syntax error, though 🙂
But I'm really glad to see this come through! 😄 |
I also previously ran into issues with this operator at #183 - I believe we chatted in the past about just dropping that symbol as the unbox operator. It's also not really needed much anymore since almost all mutable data can be handled with I'll try to land a PR tonight that removes that operator as |
Are we going to have ambiguity in situations like these for right shift operators?
|
Nope, we shouldn't have to worry about that! Speaking of which, though, we should probably stick to having only one token each for the carets... We would totally run into some issues if there was a token for |
@bmakuh feel free to rebase on master now that I landed the removal of |
9955ef0
to
347781e
Compare
@ospencer this should be ready for review now. |
347781e
to
7ccd469
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woohoo! I left one comment about the priorities, but otherwise it looks great! We might want to keep the original names of the bit-shifting operators in Pervasives in addition to the new syntax, since otherwise it'd be a breaking change. I'm going to double check with the core team though.
Excellent work!
9a64625
to
1ae17ff
Compare
@bmakuh We chatted, and we're just going to have the breaking change. I dropped your fix commit (and thank you for changing it so quickly!) and we're pretty much good to merge. |
1ae17ff
to
25d7fc6
Compare
* add bitwise operators to parser.dyp * add tokens to lexer * add operator sigils to pervasives * add tests for bitwise operators * fix: operator precedence levels
* add bitwise operators to parser.dyp * add tokens to lexer * add operator sigils to pervasives * add tests for bitwise operators * fix: operator precedence levels
* add bitwise operators to parser.dyp * add tokens to lexer * add operator sigils to pervasives * add tests for bitwise operators * fix: operator precedence levels
* add bitwise operators to parser.dyp * add tokens to lexer * add operator sigils to pervasives * add tests for bitwise operators * fix: operator precedence levels
* add bitwise operators to parser.dyp * add tokens to lexer * add operator sigils to pervasives * add tests for bitwise operators * fix: operator precedence levels
Addresses #297
Todo