-
-
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
Remove 1 item Expressions normalization #4861
Remove 1 item Expressions normalization #4861
Conversation
end | ||
|
||
it "normalizes an expression" do | ||
assert_normalize "(1 < 2).as(Bool)", "((1 < 2)).as(Bool)" |
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 are there 2 pairs of parenthesis in the normalized version?
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.
It is ASTNode#to_s
bug, but it is not related to this PR, and I've fixed it in my local repository already. I'll open a new PR.
49e82e0
to
72d785a
Compare
Hm, now I'm not convinced this change is good. This needs a bit more thought. Maybe your first way of fixing this was less obtrusive. |
@asterite I added it to support very rare case (and there is this case in while (true)
# ...
end But I think this case should not be supported now and |
@@ -1811,10 +1814,14 @@ module Crystal | |||
filter_vars cond_type_filters, &.not | |||
when Or | |||
# Try to apply boolean logic: `!(a || b)` is `!a && !b` | |||
cond_left = cond.left |
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.
Could it be a good case for refactoring? Making a helper method along the lines of:
def single_expression(cond)
if cond.is_a?(Expressions) && cond.expressions.size == 1
return cond[0]
end
end
# then you could re-use it like:
cond_left = single_expression(cond) || cond.left
# and l8r on, around line 2116
when Expressions
return unless cond = single_expression(node)
return get_while_cond_assign_target(cond)
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.
Good catch!
94886bb
to
d70d28b
Compare
@asterite ping. What do you think now? |
@makenowjust I won't have time for this |
That would be great! |
@makenowjust Sorry, but right now I'm very confused, but I think the changes are good. I think we can still let endless_while = node.cond.true_literal? We can probably change it to something like: endless_while = node.cond.true_literal? || single_expression(node.cond).try &.true_literal? (not tested) |
@asterite I can't understand why |
@makenowjust Because it works now and it's better not to introduce a breaking change if the fix is very simple :-) (and because in your code you are considering an Expressions with a single expression the same as that single expression, so we should do it in this case too) |
@mverzilli Sorry. Please re-open this! This pull request should not be closed. |
Ooops, my bad. I'll send a PR to revert, sorry for this. |
@mverzilli why not just reopen this PR? |
Yes, I can reopen but I must also revert this change because now it's merged to master |
@mverzilli "it's merged to master", where? |
@Sija I don't know, however I can't reopen (perhaps GitHub cannot allow normal contributor to reopen the issue closed by project author.) 水曜日, 20 9月 2017, 09:26午後 +09:00 from Sijawusz Pur Rahnama notifications@github.com :
…
@mverzilli why not just reopen this PR?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub , or mute the thread .
|
@mverzilli It would help if you could provide SHA of a commit which you believe is merged into the master. |
This was never merged. It was incorrectly automatically closed because of #4862 |
Oh, that's weird! Now I understand why @Sija was asking that, sorry! Anyway, it's already reopened, sorry for the noise. |
@mverzilli To be honest, I also thought this was merged when I got the "closed" mail 😅 |
So is it ready for merging? |
I think this need to be rebased first. I found a bit overcomplicated to follow with all the references across the issues 😅 Since the normalization for single expression is removed and the |
@bcardiff Correct! I think |
Fix crystal-lang#4849 See crystal-lang#4852 (comment) This contains some MainVisitor fix to follow this change, and adds specs to `spec/compiler/normalize/expressions_spec.cr`, it's a new file.
And revert src/compiler/crystal/syntax/ast.cr changes
d70d28b
to
a3d7e58
Compare
And now `while (true)` works. Thank you @asterite.
a3d7e58
to
679b6d7
Compare
Fix #4849
See #4852 (comment)
This contains some MainVisitor fix to follow this change, and adds specs to
spec/compiler/normalize/expressions_spec.cr
, it's a new file.@asterite