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 #21306; fixes #20485; don't transform yields in the var section when introducing new local vars [backport: 1.6] #21489

Merged
merged 4 commits into from
Mar 10, 2023

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Mar 8, 2023

fixes #21306
fixes #20485

introduceNewLocalVars shouldn't transform yields when introducing local variables. It doesn't do for plain yields, namely only transform their children. However it is not the same case for yields in the var sections. We should make it the same as plain yields.

reduced cases

iterator blocks(): int =
  yield 1
  yield 2

iterator push(): int {.closure.} =
  for _ in blocks():
    let res = block:
      yield 1
      1

let s = push
doAssert false

Probably it is better to set the inlining to zero when calling introduceNewLocalVars. They should have the same outcome.

@ringabout ringabout marked this pull request as ready for review March 8, 2023 07:32
@ringabout ringabout marked this pull request as draft March 8, 2023 07:52
@ringabout ringabout marked this pull request as ready for review March 8, 2023 08:38
@ringabout ringabout changed the title fixes #21306; don't transform yields in the var section when introducing new local vars [backport: 1.6] fixes #21306; fixes #20485; don't transform yields in the var section when introducing new local vars [backport: 1.6] Mar 8, 2023
@Araq Araq merged commit f2dad94 into devel Mar 10, 2023
@Araq Araq deleted the pr_yields branch March 10, 2023 13:19
@github-actions
Copy link
Contributor

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

Hint: mm: orc; opt: speed; options: -d:release
166280 lines; 11.304s; 612.5MiB peakmem

narimiran pushed a commit that referenced this pull request Mar 23, 2023
… when introducing new local vars [backport: 1.6] (#21489)

* fixes #21306;  don't transform yields in the var section when introducing new local vars

* adds `inVarSection` so the var section in the var section is freshed

* use `isIntroducingNewLocalVars` to avoid yield transformations in var sections

* fixes comments

(cherry picked from commit f2dad94)
capocasa pushed a commit to capocasa/Nim that referenced this pull request Mar 31, 2023
… the var section when introducing new local vars [backport: 1.6] (nim-lang#21489)

* fixes nim-lang#21306;  don't transform yields in the var section when introducing new local vars

* adds `inVarSection` so the var section in the var section is freshed

* use `isIntroducingNewLocalVars` to avoid yield transformations in var sections

* fixes comments
bung87 pushed a commit to bung87/Nim that referenced this pull request Jul 29, 2023
… the var section when introducing new local vars [backport: 1.6] (nim-lang#21489)

* fixes nim-lang#21306;  don't transform yields in the var section when introducing new local vars

* adds `inVarSection` so the var section in the var section is freshed

* use `isIntroducingNewLocalVars` to avoid yield transformations in var sections

* fixes comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants