Skip to content
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

fix #9503 plus another proposal on comparison and iteration syntax #15524

Merged
merged 2 commits into from
Mar 18, 2016

Conversation

JeffBezanson
Copy link
Member

The first commit is a straightforward fix for #9503, always parsing a<:b as an expression with head <:.

This change inspired the second commit, which does two related things. First, 2-argument comparisons are parsed as calls. Previously, we parsed a<b as (comparison a < b), which I think is a quasi-bug since this can be handled as a simple function call and doesn't need chained comparison treatment.

That led to looking at iteration syntax, which depends on the parsing of a in b. One problem here is that = and in have different precedence, so depending on which you used you could get different syntax errors. For example for i = a&&b parses but for i in a&&b is a syntax error. I fixed that by using in precedence in all cases.

Next I decided that it's not so good to allow for i = 1:n f(i) end, so I deprecated that. Allowing space as an implicit separator here is one thing that prevents generalizing in syntax to other function names.

@tkelman
Copy link
Contributor

tkelman commented Mar 15, 2016

First, 2-argument comparisons are parsed as calls. Previously, we parsed a<b as (comparison a < b), which I think is a quasi-bug since this can be handled as a simple function call and doesn't need chained comparison treatment.

Breaks-JuMP warning! And probably any other package that has macros that deal with comparisons.

@tkelman tkelman added the breaking This change will break code label Mar 15, 2016
@iamed2
Copy link
Contributor

iamed2 commented Mar 15, 2016

While working on https://github.com/invenia/JuliaFormat.jl I discovered the comparison special case and it surprised me. What was/is the reasoning/use for it?

@mlubin
Copy link
Member

mlubin commented Mar 15, 2016

We can handle this breakage, don't let it hold you up.

@JeffBezanson
Copy link
Member Author

Yes, everything in this PR is breaking. I think the new version is easier, since comparison chains a < b < c really are a different case than a < b. Any code that handles function calls can handle a < b, while comparison chains require inserting control flow, and might not be supported at all in a certain macro. So separating these cases more makes sense.

(t (peek-token s)))
(if (not (or (closing-token? t) (newline? t)))
;; should be: (error "invalid iteration specification")
(syntax-deprecation s (string "for " (deparse `(= ,lhs ,rhs)) " " t)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any test for the deprecated syntax?

@JeffBezanson
Copy link
Member Author

I'm going to leave the for loop separator issue out of this; belongs in a separate change I think.

JeffBezanson added a commit that referenced this pull request Mar 18, 2016
fix #9503 plus another proposal on comparison and iteration syntax
@JeffBezanson JeffBezanson merged commit a9a7bce into master Mar 18, 2016
@tkelman tkelman deleted the jb/comparisonparse branch March 18, 2016 23:19
@tkelman
Copy link
Contributor

tkelman commented Jul 2, 2016

@timholy is any change needed in exprresolve_conditional for this?

julia/base/cartesian.jl

Lines 371 to 376 in 0d5f541

function exprresolve_conditional(ex::Expr)
if ex.head == :comparison && isa(ex.args[1], Number) && isa(ex.args[3], Number)
return true, exprresolve_cond_dict[ex.args[2]](ex.args[1], ex.args[3])
end
false, false
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants