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

avoid jl_value_ptr #671

Merged
merged 2 commits into from
Mar 26, 2019
Merged

avoid jl_value_ptr #671

merged 2 commits into from
Mar 26, 2019

Conversation

stevengj
Copy link
Member

A cleaner solution to JuliaLang/julia#31473 and #670.

@yuyichao
Copy link
Collaborator

Technically this is dangerous since the argument to unsafe_convert is not guaranteed rooted by the caller here. The current implementation my or may not take advantage of this but it is technically allowed to assume that and give wrong result.. pointer_from_objref should be fine.

@stevengj
Copy link
Member Author

stevengj commented Mar 26, 2019

@yuyichao, the jlWrapType variable is a const global precisely because we can't allow it to be garbage-collected as long as we are using the tp_base pointer.

So, for the same reason, I don't think rooting is an issue for unsafe_convert?

I tend prefer this to pointer_from_objref because it seems to more clearly indicate the intent: "give me the pointer to the C-like struct for this type" rather than "give me the jl_value_t pointer". Those two happen to be the same thing at the moment, but it is not obvious to me that this will always be the case.

@stevengj
Copy link
Member Author

stevengj commented Mar 26, 2019

I guess in theory the RefValue object needs to be rooted too, so I would need a GC.@preserve here?

@yuyichao
Copy link
Collaborator

Correct, it's the

argument to unsafe_convert

that's the issue.

@stevengj stevengj merged commit 5f141ee into master Mar 26, 2019
@stevengj stevengj deleted the pytypeptr branch March 26, 2019 15:34
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