-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 &+ &- &* &** 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -600,13 +600,13 @@ describe "Code gen: proc" do | |||
)).to_i.should eq(1) | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add a lookahead to avoid this breaking change?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
peek_next_char
?
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.
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_char
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated again to revert the change in the spec that uses &->
where parentheses were added.
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.
Diff merged. Thanks @asterite
@@ -132,6 +132,7 @@ module Crystal | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is support for &+ 1
left out intentionally?
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.
No, I just added &-
because there is a prefix operation -
. I will add the specs.
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.
I asked since AFAIR there's also prefix +
operator.
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.
Can prefix -
or +
overflow? If not, maybe it's not worth adding these.
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -914,7 +914,7 @@ describe "Block inference" do | |||
)) { types["Moo"].types["Bar"] } | |||
end | |||
|
|||
it "passes &->f" do | |||
it "passes &(->f)" do |
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.
These changes should no longer be necessary.
@@ -1,8 +1,10 @@ | |||
require "../../spec_helper" | |||
|
|||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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&->proc
that 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)