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 #19401; fixes #19402; rework Forward declaration and finalizer for ORC #20295

Merged
merged 11 commits into from
Sep 27, 2022

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Sep 1, 2022

fixes #19401
fixes #19402

It is a draft because It's the solution I can think of for now. I'm going to examine it tomorrow. At least it works for #19401 and #19402 locally.

@ringabout ringabout closed this Sep 2, 2022
@ringabout ringabout reopened this Sep 2, 2022
@ringabout ringabout marked this pull request as ready for review September 2, 2022 12:21
@ringabout
Copy link
Member Author

ringabout commented Sep 2, 2022

It seems that at the time of magicsAfterOverloadResolution is called, the finalizer function is not transformed completely. By adding a wrapper around the finalizer function, we can ignore the incomplete function body and delay the analysis to later phases. Finally, when the finalizer function gets transformed properly, and the wrapper function will call it. So we don't need to touch the internal symbols of the finalizer function.

For instance

type
  Object = object
  Ref = ref Object

proc delete(x: Ref)

It generates a deleteFinalizerWrapper function:

proc delete(x: Ref)
proc deleteFinalizerWrapper(x: var Object) =
  delete(x)

After some refactorings, it can solve #19231 too

@ringabout ringabout marked this pull request as draft September 2, 2022 12:40
@ringabout ringabout closed this Sep 2, 2022
@ringabout ringabout reopened this Sep 2, 2022
@Varriount Varriount requested a review from Araq September 4, 2022 18:53
@Varriount Varriount added the Requires Araq To Merge PR should only be merged by Araq label Sep 4, 2022
compiler/semmagic.nim Outdated Show resolved Hide resolved
@ringabout ringabout mentioned this pull request Sep 15, 2022
@Araq Araq merged commit 80e739f into devel Sep 27, 2022
@Araq Araq deleted the pr_arc_finalizer branch September 27, 2022 18:07
@github-actions
Copy link
Contributor

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

Hint: mm: orc; threads: on; opt: speed; options: -d:release
164214 lines; 17.767s; 667.836MiB peakmem

capocasa pushed a commit to capocasa/Nim that referenced this pull request Mar 31, 2023
…n and finalizer for ORC (nim-lang#20295)

* fixes nim-lang#19401; fixes nim-lang#19402; rework Forward declaration and finalizer for ORC

* add more tests

* give it a name

* make more tests

* fixes tests

* hidden addr for cpp

* move code to a function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Requires Araq To Merge PR should only be merged by Araq
Projects
None yet
3 participants