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

No need for localvar with Julia >= 1.0 #1808

Open
wants to merge 3 commits into
base: release-0.18
from

Conversation

@xhub
Copy link

commented Jan 23, 2019

As noted in
#1216 (comment)
there is no need for localvar anymore.

This change is need for the EMP.jl package, in which I have macros that are calling JuMP macros. Without this fix, the JuMP macros is trying to create a local variable like EMP:.##43## with Julia >= 0.7, and it fails. With Julia 0.6, the code works fine.

There may be another way to fix this, but removing the function call seems to be the way forward.

This pass all tests. If this change is accepted, I'll do a PR for the master branch ( where localvar is also marked as removable, see https://github.com/JuliaOpt/JuMP.jl/blob/master/src/macros.jl#L192 )

No need for localvar with Julia >= 0.7
As noted in
#1216 (comment)
there is no need for localvar anymore.
@blegat
blegat approved these changes Jan 23, 2019
Copy link
Member

left a comment

Looks good. For master, you can remove this function entirely since we dropped Julia v0.6

@xhub

This comment has been minimized.

Copy link
Author

commented Jan 23, 2019

Thanks, will do.

@xhub

This comment has been minimized.

Copy link
Author

commented Jan 23, 2019

Need to have a look at the failures ...

@odow

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

This change is need for the EMP.jl package, in which I have macros that are calling JuMP macros.

I also had this problem with SDDP.jl! It blocked me from upgrading it to Julia 1.0. I tried a few things but got confused with all the scoping/hygiene issues so never tracked down the root cause. I'll be pleased if this works!

@xhub

This comment has been minimized.

Copy link
Author

commented Jan 23, 2019

Hm ... I need to better with the new Pkg, I tested the wrong version (at least on 0.7).

So the change of behaviour happened in 1.0, not 0.7, as one can see with

toto = 4
for inner toto=1:3
       println(toto)
 end
println(toto)

Therefore, I'm going to update the version check to < v"1.0" and call it a day, I am fine with my software not working on 0.7.

@xhub

This comment has been minimized.

Copy link
Author

commented Jan 23, 2019

I also had this problem with SDDP.jl! It blocked me from upgrading it to Julia 1.0. I tried a few things but got confused with all the scoping/hygiene issues so never tracked down the root cause. I'll be pleased if this works!

I made it work for my use case. I still need to polish one aspect, I'll ping you when I am done

@xhub xhub changed the title No need for localvar with Julia >= 0.7 No need for localvar with Julia >= 1.0 Jan 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.