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

fixes isLocalVarSym; an implicit global is a global nonetheless #21025

Merged
merged 1 commit into from
Dec 5, 2022

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Dec 5, 2022

ref #20812

The definition of isLocalVarSym is not correct. I found it when I was debugging #21024. I tried to reuse this function, but it gave surprising results.

For a global variable, it doesn't need both sfGlobal and sfPure flags which indicates it is an explicit global variable because it can be an implicit global variable without a sfPure flag. I expect this function identifies all global variables. So I change its definition.

It should still stand correct for tests in #20812 because the previous PR used a guard statement even it was based on a wrong requirement.

  if {sfGlobal, sfPure} <= v.flags:
    globalVarInitCheck(c, def)

@ringabout ringabout changed the title fixes isLocalVarSym; an implicit global is a global nonetheless fixes isLocalVarSym; an implicit global is a global nonetheless Dec 5, 2022
@bung87
Copy link
Collaborator

bung87 commented Dec 5, 2022

all the code writen there don't expect implicit global variable and only used for globalVarInitCheck, your patch #21024 widen its useage.

@Araq
Copy link
Member

Araq commented Dec 5, 2022

Why is it not sfGlobal in v.flags then?

@bung87
Copy link
Collaborator

bung87 commented Dec 5, 2022

it consistent with compiler/injectdestructors.nim

if {sfGlobal, sfPure} <= v.sym.flags or sfGlobal in v.sym.flags and s.parent == nil: 
  c.graph.globalDestructors.add c.genDestroy(v)

and variables are really declared in global scope.

@Araq Araq merged commit a8090f7 into devel Dec 5, 2022
@Araq Araq deleted the pr_local_var branch December 5, 2022 21:24
@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2022

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from a8090f7

Hint: mm: orc; opt: speed; options: -d:release
165502 lines; 8.057s; 613.902MiB peakmem

survivorm pushed a commit to survivorm/Nim that referenced this pull request Feb 28, 2023
…m-lang#21025)

fixes isLocalVarSym; an implicit global is a global nonetheless
capocasa pushed a commit to capocasa/Nim that referenced this pull request Mar 31, 2023
…m-lang#21025)

fixes isLocalVarSym; an implicit global is a global nonetheless
narimiran pushed a commit that referenced this pull request May 12, 2023
…1025)

fixes isLocalVarSym; an implicit global is a global nonetheless

(cherry picked from commit a8090f7)
bung87 pushed a commit to bung87/Nim that referenced this pull request Jul 29, 2023
…m-lang#21025)

fixes isLocalVarSym; an implicit global is a global nonetheless
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.

3 participants