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

Created ScopeData for Julia's Scope #3890

Merged
merged 5 commits into from
Jul 28, 2024
Merged

Created ScopeData for Julia's Scope #3890

merged 5 commits into from
Jul 28, 2024

Conversation

B-rando1
Copy link
Collaborator

@B-rando1 B-rando1 commented Jul 26, 2024

I created ScopeData, which is intended to hold information about a variable's scope. I also switched Julia's ScopeSym 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

@B-rando1 B-rando1 force-pushed the GlobalLocalScope2 branch 2 times, most recently from 51e8dc2 to d46d770 Compare July 26, 2024 20:37
Copy link
Owner

@JacquesCarette JacquesCarette left a 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`.
@B-rando1
Copy link
Collaborator Author

You're right, this should have been on its own branch from the start. I've rebased it to main.

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.
@B-rando1
Copy link
Collaborator Author

I added ScopeData to the other renderers as well, abstracting the functions into their appropriate places. I was initially hoping that we could just use ScopeData in the Julia renderer where it's used, but it looks like because we need it in the state, all of the renderers need to use the same type.

@B-rando1
Copy link
Collaborator Author

I removed the Doc from ScopeData. Initially I was thinking it might be handy to add flexibility for the keyword used in global/local declarations, but now I'm thinking that might be premature. We only have one language that does these declarations, and it's hard to say if that would actually be handy if we do add another language with them.

Because the functions are all mostly shared between renderers, it would be easy to add another field to ScopeData if we want to add one later.

@JacquesCarette JacquesCarette merged commit bd3b1c1 into main Jul 28, 2024
5 checks passed
@JacquesCarette JacquesCarette deleted the GlobalLocalScope2 branch July 28, 2024 10:17
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