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

add init, del, repr, doc for pyType #308

Merged
merged 6 commits into from
Jan 22, 2025
Merged

add init, del, repr, doc for pyType #308

merged 6 commits into from
Jan 22, 2025

Conversation

YesDrX
Copy link
Contributor

@YesDrX YesDrX commented Jan 18, 2025

Hi @yglukhov , please pull the following merge request.

implement init, del, doc for exported types

Example

fix the CI/CD pipeline

Logs

made some minor changes on the readme file

Copy link
Owner

@yglukhov yglukhov left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Destroy hooks should already work with regular nim machinery, i.e. =destroy() hooks. If they don't it should be fixed, but no additional conventions should be invented.

Also please use 2 spaces indentation wherever you're using 4.

docs/export_python_type.md Outdated Show resolved Hide resolved
tests/texport_pytype.nim Show resolved Hide resolved
@YesDrX
Copy link
Contributor Author

YesDrX commented Jan 18, 2025

Thanks for the PR!

Destroy hooks should already work with regular nim machinery, i.e. =destroy() hooks. If they don't it should be fixed, but no additional conventions should be invented.

Also please use 2 spaces indentation wherever you're using 4.

I saw that you bind tp_free, tp_dealloc here

nimpy/nimpy.nim

Lines 154 to 160 in 0645af5

ty.tp_basicsize = td.origSize.cint - sizeof(pointer).cint
ty.tp_flags = defaultTPFLAGS()
ty.tp_doc = td.doc
ty.tp_new = td.newFunc
ty.tp_free = freeNimObj
ty.tp_dealloc = destructNimObj
ty.tp_descr_get = typDescrGet

However, tp_finalize is a separate function (called before tp_dealloc) (del in python). If someone has some extra memory management need be done before destroy the object. tp_finalize is the function to call.

@YesDrX YesDrX requested a review from yglukhov January 18, 2025 20:36
@yglukhov
Copy link
Owner

I don't understand what's so valuable in the finalize callback vs nim destructors that justifies it, despite the fact that
a. it's not guaranteed to work in all python environments (only newer python versions)
b. it will never be called from the nim side if the object never leaves nim boundary.
c. object can leave python runtime, triggering finalize, but then continue living perfectly "valid" on the nim side, but with finalizer already called
All of the scenarios above are just an awful mess. What's the advantage? What's the "extra memory management" you can only do in finalize but not in destructor?

@YesDrX
Copy link
Contributor Author

YesDrX commented Jan 21, 2025

I don't understand what's so valuable in the finalize callback vs nim destructors that justifies it, despite the fact that a. it's not guaranteed to work in all python environments (only newer python versions) b. it will never be called from the nim side if the object never leaves nim boundary. c. object can leave python runtime, triggering finalize, but then continue living perfectly "valid" on the nim side, but with finalizer already called All of the scenarios above are just an awful mess. What's the advantage? What's the "extra memory management" you can only do in finalize but not in destructor?

Ok. __del__ is now removed.

@yglukhov yglukhov merged commit 114e1b9 into yglukhov:master Jan 22, 2025
@yglukhov
Copy link
Owner

Thank you!

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