Skip to content

push block label sym owner for const and static #24085

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

Closed
wants to merge 4 commits into from

Conversation

metagn
Copy link
Collaborator

@metagn metagn commented Sep 8, 2024

fixes #8758, fixes #10828, fixes #12172, fixes #21610, fixes #23803, refs nim-lang/RFCs#276

Based off of #24084 for now but maybe doesn't need it

Another option is to reuse the instantiatedFrom field

@metagn
Copy link
Collaborator Author

metagn commented Sep 8, 2024

As expected the compiler doesn't like that the owner of a symbol needs skips to get to the owning proc, we could add the skips in the required places but that might hurt performance and would take a lot of effort.

For skVar and skLet we have the advantage that they will never use the s.instantiatedFrom symbol, so we can track the "owner" in another field in PContext and give that to instantiatedFrom. Unfortunately skTemp symbols also use sfFromGeneric for some reason (for at least 15 years) so I'm not sure if that would trip the compiler up in some places (seems to just be used to skip semming though). Maybe it already trips the compiler up for skVar and skLet too.

I'll test using instantiatedFrom for now, if it's not considered too hacky I'll polish it.

@metagn
Copy link
Collaborator Author

metagn commented Feb 7, 2025

Another option might be to track the introduced variables in vmgen and only allow them to be used

Edit: Done in #24674

metagn added a commit to metagn/Nim that referenced this pull request Feb 8, 2025
@metagn metagn closed this Feb 8, 2025
Araq pushed a commit that referenced this pull request Feb 14, 2025
fixes #8758, fixes #10828, fixes #12172, fixes #21610, fixes #23803,
fixes #24633, fixes #24634, succeeds #24085

We simply track the symbol ID of every traversed `var`/`let` definition
in `vmgen`, then these symbols are always considered evaluable in the
current `vmgen` context. The set of symbols is reset before every
generation, but both tests worked properly without doing this including
the nested `const`, so maybe it's already done in some way I'm not
seeing.
narimiran pushed a commit that referenced this pull request Feb 17, 2025
fixes #8758, fixes #10828, fixes #12172, fixes #21610, fixes #23803,
fixes #24633, fixes #24634, succeeds #24085

We simply track the symbol ID of every traversed `var`/`let` definition
in `vmgen`, then these symbols are always considered evaluable in the
current `vmgen` context. The set of symbols is reset before every
generation, but both tests worked properly without doing this including
the nested `const`, so maybe it's already done in some way I'm not
seeing.

(cherry picked from commit a5cc33c)
narimiran pushed a commit that referenced this pull request Mar 3, 2025
fixes #8758, fixes #10828, fixes #12172, fixes #21610, fixes #23803,
fixes #24633, fixes #24634, succeeds #24085

We simply track the symbol ID of every traversed `var`/`let` definition
in `vmgen`, then these symbols are always considered evaluable in the
current `vmgen` context. The set of symbols is reset before every
generation, but both tests worked properly without doing this including
the nested `const`, so maybe it's already done in some way I'm not
seeing.

(cherry picked from commit a5cc33c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant