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

Add unique_id field to Var #29

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cscherrer
Copy link
Contributor

Closes #28

For demonstration, I temporarily added the @show line here:

    function local_var_to_var(var::LocalVar)::Var
        unique_id = Symbol(var.sym, "_", id[var.sym])
        @show unique_id # Remove after demo
        Var(var.sym, unique_id, var.is_mutable[], var.is_shared[], false)
    end

Then for example,

julia> quote
           x = 1
           function (a)
               x = 1
           end
       end |>  solve_from_local
unique_id = :x_1
unique_id = :x_2
unique_id = :a_1
unique_id = :x_2
unique_id = :x_2
:($(Expr(:scoped, (bounds = Var[mut @shared x], freevars = Var[], bound_inits = Symbol[]), quote
    #= REPL[19]:2 =#
    mut @shared x = 1
    #= REPL[19]:3 =#
    function (a,)
        $(Expr(:scoped, (bounds = Var[@local a], freevars = Var[mut @shared x], bound_inits = [:a]), quote
    #= REPL[19]:3 =#
    #= REPL[19]:4 =#
    mut @shared x = 1
end))
    end
end)))

This PR changes Var to

struct Var
    name::Symbol
    unique_id::Symbol
    is_mutable::Bool
    is_shared::Bool
    is_global::Bool
end

The unique_id field is not displayed, but can easily be used downstream for refactoring local variables.

@cscherrer
Copy link
Contributor Author

@thautwarm Any concerns on this other than cscherrer#1?

@thautwarm
Copy link
Member

I prefer cscherrer#1 . Having symbols as the umique_id works for limited cases and makes it hard to compose scoped expr correctly.

Consider you call solve! on different Exprs, and you want to use them together. Symbols might appear in the same scope, and two from different exprs can have the same name. This is however dangerous. Or you have to maintain a symbol generator that gives globally unique names.

You might prefer Symbols as unique ids for your use case, but it may be done without pains via id_function?

@cscherrer
Copy link
Contributor Author

cscherrer commented Mar 29, 2022

I think we're talking past each other. This PR has unique_id:: Union{Ref{Bool}, Nothing}. cscherrer#1 is a PR into this one, that adds an id_function to make it more general.

@cscherrer
Copy link
Contributor Author

You said

The only issue is the compatibility. Merging it could be bring breaking changes, and I'm not ready for it.

which I understood to mean that you prefer this PR, without the id_function option

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can Var have an id field?
2 participants