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 id_function #1

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

add id_function #1

wants to merge 3 commits into from

Conversation

cscherrer
Copy link
Owner

@cscherrer cscherrer commented Mar 17, 2022

This allows solve! to take a function argument, so now it's solve!(id_function, ast; toplevel=true). Here id_function is a function of two arguments,

id_function(::Ref{Bool}, ::Symbol)

If the function is left out, it's taken to be (a, _) -> a

@cscherrer
Copy link
Owner Author

Any thoughts on this @thautwarm? Using a Ref as an ID is efficient, but there are a lot of cases where we'll need something different. We could walk the AST after calling solve!, but it would be nice to avoid this extra overhead if possible.

If you like this idea I could merge it here, so it would be added to the PR JuliaStaging#29

@thautwarm
Copy link

thautwarm commented Mar 29, 2022

I think such design is good: it gets decoupled from the design mistake of NameResolution.jl.

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

Maybe you can use different Var when id_function is specified, and the default id_function create scoped Expr with the original Var?

@cscherrer
Copy link
Owner Author

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

That's ok. I can work around it, this was just an idea for some added convenience.

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.

2 participants