-
Notifications
You must be signed in to change notification settings - Fork 25
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
Created ScopeData
for Julia's Scope
#3890
Conversation
51e8dc2
to
d46d770
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this seems reasonable, as it is built on top of a PR that I'm not sure is the right way to go, I won't approve until that is investigated.
This change is quite likely independent of that one, and should likely have been done that way.
In `drasil-code`, I should be done assigning all variables their proper scope. There's definitely a good chance I made some mistakes, but hopefully we can spot them during testing.
Currently nothing is done with it, but I _think_ it's a step in the right direction of actually using `Scope`.
d46d770
to
d6b8042
Compare
You're right, this should have been on its own branch from the start. I've rebased it to |
It's still just being thrown away at this point, but the next step is to add it to `VarData` so that it is kept in the state.
Initially I thought it might be handy in the future to have the added flexibility of using a different keyword for `global`/`local` declarations, but now I'm thinking that might be a little bit too premature. Only one of our renderers uses `ScopeSym`, and when we add another one we don't know what will be handy to have in `ScopeData`. We can discuss this more in the PR.
I added
|
I removed the Because the functions are all mostly shared between renderers, it would be easy to add another field to |
This reverts commit 5f2d1eb.
I created
ScopeData
, which is intended to hold information about a variable's scope. I also switched Julia'sScopeSym
representation to use it.Currently nothing is done with it, but I believe we need to integrate it into the AST for Julia, so that we can get the information back out of the variable as needed.
Contributes to #3821