Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 using cfunction runtime closures in iteration callback #812
Avoid using cfunction runtime closures in iteration callback #812
Changes from all commits
63695fd
44c7d4b
d75c031
52a6cc2
b1125d4
7b3778f
afe843e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it matter that this is a
Ref
instead of aPtr{Any}
thereby eliminating the unsafe_convert below? PresumablyRef
is the better option here sincef
is a method defined on the Julia end, which more closely aligns with the general usage of Ref.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are two issues here:
Base.pointer_from_objref
.::Any
and thus have Julia take care of the implicit conversionI am a bit sketched out by the
Ptr{Any}
which then is converted toAny
on the callee side,but I thinkRef{Any}
inccall
/cfunction
has special handling.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm a bit unsure of the dance of
Ptr{Any}
,Ref{Any}
,Any
(and having put this up over two weeks ago, I forget what some of my problems/conclusions were). I think it's the special-cased handling mentioned in theRef
docstring that was throwing me off for a while, and this is what I got to work:Since @vtjnash just commented here, though, would you mind giving an expert opinion on what the cleanest way to do this would be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simply put, you need to use the same expression in both places. That can be
Ref{T}
orAny
(or perhaps evenRef{Any}
, but that seems like usually an odd choice)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I simplified the signature in the
@cfunction
call and then the argument being passed through to the low-levelh5(a|l)_iterate
. (Not sure how I skipped over that particular variation a couple of weeks ago considering it is the simple option...)If I'm understanding correctly, though, I do still need the
userf = Ref{Any}(f)
line so that the closure has a pointer address for theccall
to make use of.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need that? You've used
cfunction
withAny
, so you must useccall
withAny
alsoThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the
Ref
container, there are MethodErrors due to nounsafe_convert{::Type{Ptr{Nothing}}, ::<closure type>)
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, I misunderstood and answered the wrong question.
We've been following the C api, so the ccall wrapper function's are declared with
void*
/Ptr{Cvoid}
arguments. Are you saying we should change that toAny
instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is the wrong decl.
This is intentional, as otherwise you're missing the GC root (as detected by the miscompilation assert)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly this is causing issues on 1.4