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

RFC: fix #42345 (duplicate of #34862): Update to calling-c-and-fortran-code.md #42480

Closed
wants to merge 1 commit into from

Conversation

danphenderson
Copy link
Contributor

@danphenderson danphenderson commented Oct 3, 2021

Documentation issue #42345 was not present in v1.2.0 and first appears in v1.3.0.

I initially presumed the documentation accidentally got deleted, however, reading v1.2.0 docs the chopped paragraph appears to have been rightfully updated. (at least from what I gathered reading various releases of calling-c-and-fortran-code.md). Please review and ensure accuracy - This was new information for me.

In summary, this pull request contains a condensing of the paragraph beneath the reported issue and a deleting of the extraneous/duplicated information.

@Gnimuc
Copy link
Contributor

Gnimuc commented Oct 3, 2021

Isn't it the best practice to always use Ptr{} in the input signature of ccall when calling C?

BTW, I don't think it is necessary to strictly type p in the function signature: permutation_free(p::Ref{gsl_permutation}).

@kshyatt kshyatt added the docs This change adds or pertains to documentation label Oct 4, 2021
@danphenderson
Copy link
Contributor Author

danphenderson commented Oct 4, 2021

Isn't it the best practice to always use Ptr{} in the input signature of ccall when calling C?

According to the documentation section, When to use T, Ptr{T} and Ref{T}, it is not always best to use Ptr{T} in the input signature of the ccall. However, in the ccall in the body of permutation_free, it is preferable to pass Ptr{T}. This is because the only input that should be passed is a C-controlled pointer obtained from permutation_alloc (and accepting anything else will cause the wrapped routine to free a Julia-managed memory segment).

Now consider the case of populating a Julia-managed segment of memory by means of an external C routine, then the ccall should accept Ref{T}.

BTW, I don't think it is necessary to strictly type p in the function signature: permutation_free(p::Ref{gsl_permutation}).

As for this comment, I disagree. Your suggestion would be inconsistent with every other example provided in the calling-c-and-fortran-code.md.

@Gnimuc
Copy link
Contributor

Gnimuc commented Oct 5, 2021

According to the documentation section, When to use T, Ptr{T} and Ref{T}, it is not always best to use Ptr{T} in the input signature of the ccall.

The doc is not always right and sometimes can be quite misleading. I guess you're referencing the following statement:

For C code accepting pointers, Ref{T} should generally be used for the types of input arguments, allowing the use of pointers to memory managed by either Julia or C through the implicit call to Base.cconvert.

To me, this statement implies that if Ptr{T} is used as the types of input arguments, then when passing a pointer to memory managed by Julia(e.g. a Ref), Base.cconvert may not work(?).

Why should users care about this at all? According to the following conversion methods defined in Base, Base.cconvert will defer the conversion to Base.unsafe_convert for Ptr{T}. As long as Base.unsafe_convert does the right job, using Ptr{T} should be fine. So, is there really an advantage of using Ref{T} over Ptr{T} in general? I doubt.

cconvert(T::Type, x) = convert(T, x) # do the conversion eagerly in most cases
cconvert(::Type{<:Ptr}, x) = x # but defer the conversion to Ptr to unsafe_convert
unsafe_convert(::Type{T}, x::T) where {T} = x # unsafe_convert (like convert) defaults to assuming the convert occurred
unsafe_convert(::Type{T}, x::T) where {T<:Ptr} = x  # to resolve ambiguity with the next method
unsafe_convert(::Type{P}, x::Ptr) where {P<:Ptr} = convert(P, x)
function unsafe_convert(P::Union{Type{Ptr{T}},Type{Ptr{Cvoid}}}, b::RefValue{T})::P where T
    if allocatedinline(T)
        p = pointer_from_objref(b)
    elseif isconcretetype(T) && ismutabletype(T)
        p = pointer_from_objref(b.x)
    else
        # If the slot is not leaf type, it could be either immutable or not.
        # If it is actually an immutable, then we can't take it's pointer directly
        # Instead, explicitly load the pointer from the `RefValue`,
        # which also ensures this returns same pointer as the one rooted in the `RefValue` object.
        p = pointerref(Ptr{Ptr{Cvoid}}(pointer_from_objref(b)), 1, Core.sizeof(Ptr{Cvoid}))
    end
    return p
end
function unsafe_convert(::Type{Ptr{Any}}, b::RefValue{Any})::Ptr{Any}
    return pointer_from_objref(b)
end

However, in the ccall in the body of permutation_free, it is preferable to pass Ptr{T}. This is because the only input that should be passed is a C-controlled pointer obtained from permutation_alloc (and accepting anything else will cause the wrapped routine to free a Julia-managed memory segment).

This is exactly a counterexample to the above statement about Ref{T}. It's also one of the reasons why I think the best practice is to always use Ptr{T}.

BTW, I don't think it is necessary to strictly type p in the function signature: permutation_free(p::Ref{gsl_permutation}).

As for this comment, I disagree. Your suggestion would be inconsistent with every other example provided in the calling-c-and-fortran-code.md.

As the mantra goes, "strictly type your types, loosely type your functions". I said it's not "necessary". In the case of wrapper functions, strictly type the functions may prevent some user-defined Base.unsafe_convert rules from happening.

@danphenderson danphenderson changed the title Fix #42345: Updated calling-c-and-fortran-code.md (RFC) Fix #42345 (Duplicate of #34862): Updated calling-c-and-fortran-code.md (RFC) Oct 5, 2021
@danphenderson danphenderson changed the title Fix #42345 (Duplicate of #34862): Updated calling-c-and-fortran-code.md (RFC) RFC: fix #42345 (duplicate of #34862): Update to calling-c-and-fortran-code.md Oct 5, 2021
@KristofferC
Copy link
Member

I think this can be closed based on #45206

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs This change adds or pertains to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants