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

Nim2 update #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

JorySchossau
Copy link

@JorySchossau JorySchossau commented Dec 18, 2023

I'm updating some old code using this.

Changes:

  • replaces finalizer mechanics with Nim 2 destructors.

Blocking:

Copy link
Collaborator

@mratsim mratsim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Copy link
Collaborator

@mratsim mratsim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking more closely, the upstream C bindings should not be overloaded with destructors, those should be added to the Int type here.

See rationale in subsetpark/nim-gmp#2

@JorySchossau
Copy link
Author

I agree with you. Adding destructor to Int was actually my first attempt, and the wall I hit was of course since Int is actually a ref and you can't have destructors for refs. So I will go back to my overhaul code to make the better Int and ensure it is used correctly across all the code everywhere it was dereferenced. Suggestions welcome since I may be overthinking it.

@JorySchossau
Copy link
Author

I'll have to get back to this later. Here's my progress - there are very few changes. It's probably 99% done - not sure why it's segfaulting and don't have time to dig into it at the moment. I cheated and used a template to bypass modifying any dereferencing code yet, but would be happy to remove it and roll it out across the code later once everything is working.

@JorySchossau
Copy link
Author

JorySchossau commented Dec 28, 2023

Okay, that was... educational about the defunct finalizers.

  • wrapped the underlying type in an object
  • kept the use of ref for consistency and speed
  • replaced finalizers with =destroys for the underlying object type
  • Bignum Int type now tracks if it is a "view" of a piece of Bignum Rat so the dtor won't release memory the Int does not own (something that finalizers actually solved somewhat nicely)

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