Add &+ &- &* &** operators parsing#6329
Conversation
Assignment operators are supported for &+= &-= &*=
| it "normalizes var +=" do | ||
| assert_normalize "a = 1; a += 2", "a = 1\na = a + 2" | ||
| ["+", "-", "*", "&+", "&-", "&*"].each do |op| | ||
| it "normalizes var +=" do |
There was a problem hiding this comment.
Might be a good opportunity to update the description to "normalizes var #{op}="
|
Can we get formatter specs on these? |
|
Yes, I was up to do that in other PR, but can do it here... |
spec/compiler/codegen/proc_spec.cr
Outdated
| end | ||
|
|
||
| it "passes proc as &-> to method that yields" do | ||
| it "passes proc as &(->expr) to method that yields" do |
There was a problem hiding this comment.
Why not add a lookahead to avoid this breaking change?
There was a problem hiding this comment.
Because the current lexer does not do lookahead AFAIK. So is changing a bit the nature of it for this case. And that syntax is not wide used.
There was a problem hiding this comment.
You can use this diff:
diff --git a/spec/compiler/codegen/proc_spec.cr b/spec/compiler/codegen/proc_spec.cr
index bee6aa1f0..4e08c075c 100644
--- a/spec/compiler/codegen/proc_spec.cr
+++ b/spec/compiler/codegen/proc_spec.cr
@@ -600,13 +600,13 @@ describe "Code gen: proc" do
)).to_i.should eq(1)
end
- it "passes proc as &(->expr) to method that yields" do
+ it "passes proc as &->expr to method that yields" do
run(%(
def foo
yield
end
- foo &(->{ 123 })
+ foo &->{ 123 }
)).to_i.should eq(123)
end
diff --git a/spec/compiler/parser/parser_spec.cr b/spec/compiler/parser/parser_spec.cr
index 3493e4c83..168b18344 100644
--- a/spec/compiler/parser/parser_spec.cr
+++ b/spec/compiler/parser/parser_spec.cr
@@ -1085,6 +1085,8 @@ module Crystal
it_parses "->foo=", ProcPointer.new(nil, "foo=")
it_parses "foo = 1; ->foo.foo=", [Assign.new("foo".var, 1.int32), ProcPointer.new("foo".var, "foo=")]
+ it_parses "foo &->bar", Call.new(nil, "foo", block_arg: ProcPointer.new(nil, "bar"))
+
it_parses "foo.bar = {} of Int32 => Int32", Call.new("foo".call, "bar=", HashLiteral.new(of: HashLiteral::Entry.new("Int32".path, "Int32".path)))
it_parses "alias Foo = Bar", Alias.new("Foo", "Bar".path)
diff --git a/src/compiler/crystal/syntax/lexer.cr b/src/compiler/crystal/syntax/lexer.cr
index c02bc8364..915ab8da7 100644
--- a/src/compiler/crystal/syntax/lexer.cr
+++ b/src/compiler/crystal/syntax/lexer.cr
@@ -596,11 +596,18 @@ module Crystal
@token.type = :"&+"
end
when '-'
- case next_char
- when '='
- next_char :"&-="
+ # Check if '>' comes after '&-', making it '&->'.
+ # We want to parse that like '&(->...)',
+ # so we only return '&' for now.
+ if peek_next_char == '>'
+ @token.type = :"&"
else
- @token.type = :"&-"
+ case next_char
+ when '='
+ next_char :"&-="
+ else
+ @token.type = :"&-"
+ end
end
when '*'
case next_charThere was a problem hiding this comment.
Update: I just edited the diff because you can call peek_next_char instead of reader.peek_next_char
There was a problem hiding this comment.
Updated again to revert the change in the spec that uses &-> where parentheses were added.
| it_parses "~ 1", Call.new(1.int32, "~") | ||
| it_parses "1 && 2", And.new(1.int32, 2.int32) | ||
| it_parses "1 || 2", Or.new(1.int32, 2.int32) | ||
| it_parses "&- 1", Call.new(1.int32, "&-") |
There was a problem hiding this comment.
Is support for &+ 1 left out intentionally?
There was a problem hiding this comment.
No, I just added &- because there is a prefix operation -. I will add the specs.
There was a problem hiding this comment.
I asked since AFAIR there's also prefix + operator.
There was a problem hiding this comment.
Can prefix - or + overflow? If not, maybe it's not worth adding these.
There was a problem hiding this comment.
@asterite - can on 2 complement as 0 is taken from the positive side. Dunno about systems with other bases.
There was a problem hiding this comment.
Note that I am not planning on implementing prefix &-, but since unitary - is allowed, &- should also work. It is only used in an stdlib string operation that can be replaced by 0 &- expr
|
All review comments addressed |
|
Ready for another review round |
|
@bcardiff Isn't it still a breaking change as the old operators will raise if overflow happen when they didn't before? Ah, or no. This is just the parsing part. Never mind. |
spec/compiler/semantic/block_spec.cr
Outdated
| end | ||
|
|
||
| it "passes &->f" do | ||
| it "passes &(->f)" do |
There was a problem hiding this comment.
These changes should no longer be necessary.
| describe "Normalize: op assign" do | ||
| it "normalizes var +=" do | ||
| assert_normalize "a = 1; a += 2", "a = 1\na = a + 2" | ||
| ["+", "-", "*", "&+", "&-", "&*"].each do |op| |
There was a problem hiding this comment.
seem like in the rest of this file each test case is broken out. I think it might make it easier to understand if they are different test cases.
Assignment operators are supported for
&+=&-=&*=.Since
**=is not currently supported, neither is&**=.Since
&-collide with&->procthat expression needs to be written as&(->proc)after this PR. Unless the lexer is changed to add lookahead in that case there is a slightly breaking change here.I'm fine with this breaking change.
Ref: this is part of the follow up of #6223 (comment)