Improve #4708 #4852
Improve #4708 #4852
Conversation
This fixes `ToSVisitor` uses parent node information to decide to wrap receiver with parens (so we can detect chained comparison correctly!) `to_s` with `1 <= 2 <= 3` AST yields itself, and now `to_s` with `(1 <= 2).as(Bool)` AST yields itself too.
makenowjust
changed the title
to_s to fix #4708There are difference between `(1 <= 2) <= 3` and `1 <= 2 <= 3`, first is comparison of comparison, but second is chained comparison. This fix is need to work `pp (1 <= 2) <= 3` correctly (this code should causes compilation error such like `Bool has no '<=' method` and I believe this behavior is right.).
makenowjust
force-pushed
the
fix/crystal/to-s-comparison-correctly
branch
from
5a0d095
to
fd378d2
Compare
|
@makenowjust Maybe instead we can leave things as they are now, but change the normalize of I'd also add a test for the |
|
@asterite It's interesting. But I have fundamental question: is chained comparison really needed? Crystal has
Yes. But where is this test placed? (We may need to add |
|
We can probably removed chained comparison, yes. I added it in the past because it bothered me to write So for me, yes, let's remove chained comparison. I'd like to know what others think, though. |
|
Please leave it in, |
|
Yes, let's leave it in :-) |
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.
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.
|
What is the state of this? Should it be closed or merged? |
|
This shouldn't be merge, #4861 is better. I think, I'm so confused with so many PRs targetting the same issue. |
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.
|
@makenowjust can we close then? |
|
@mverzilli of course welcome |
Fix #4849
By this fix,
ToSVisitoruses parent node information to decide to wrap receiver with parens, so we can detect chained comparison correctly!to_swith1 <= 2 <= 3AST yields itself, and nowto_swith(1 <= 2).as(Bool)AST yields itself too.