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

Update calling-c-and-fortran docs #31700

Merged
merged 7 commits into from
Apr 17, 2019

Conversation

kmsquire
Copy link
Member

This is an updated version of #10799. It's almost entirely formatting and wording updates.

Comments welcome, although I probably won't be able to spend much time on this.

Maintainers should feel free to push edits.

@Gnimuc
Copy link
Contributor

Gnimuc commented Apr 12, 2019

Maybe we can also remove some misleading docs in this PR as per discussed in JuliaInterop/Clang.jl#120 (comment) ?

@Gnimuc
Copy link
Contributor

Gnimuc commented Apr 14, 2019

Note that for this code to work correctly, result_array must be declared to be of type Ref{Cdouble} and not Ptr{Cdouble}. The memory is managed by Julia and the Ref signature alerts Julia's garbage collector to keep managing the memory for result_array while the ccall executes.

I suggest to rephrase or remove the above misleading statements. It just throws some "best practice" rules and doesn't explain the mechanism under the hood.

result_array must be declared to be of type Ref{Cdouble} and not Ptr{Cdouble}.

This is not right. Ptr{Cdouble} should also work safely as long as Base.cconvert(Ptr{Cdouble}, result_array) doesn't violate the mechanism. Currently, it calls Base.cconvert(::Type{<:Ptr}, x) = x where the return value x is guaranteed to be alive (GC-preserved) until the end of ccall.

the Ref signature alerts Julia's garbage collector to keep managing the memory for result_array while the ccall executes.

Again, this is not how it works. Ref is not being treated specially, instead, there are some default cconvert&unsafe_convert methods pre-defined in Julia base, which ensures its GC safety.

BTW, I think we'd better mention more use cases for GC.@preserve in the doc.


```julia
ccall((:foo, "libfoo"), Cvoid, (Int32, Float64),
Base.unsafe_convert(Int32, Base.cconvert(Int32, x)),
Base.unsafe_convert(Float64, Base.cconvert(Float64, y)))
Base.convert(Int32, x), Base.convert(Float64, y))
Copy link
Member

Choose a reason for hiding this comment

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

Why change this and remove the text below? This is not how it works and gives the wrong impression.

Copy link
Member Author

@kmsquire kmsquire Apr 15, 2019

Choose a reason for hiding this comment

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

The text below is preserved as a footnote. I wrote it this way because I felt that, for most users, cconvert followed by unsafe_convert was more detail than they would want to know, and that this gets the point across (i.e., that the passed values are converted to the appropriate type).

That said, I'm can change it back if you don't think the footnote is enough or appropriate.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, this is the docs for how to ccall so why should I have to scroll to a footnote in order to see how ccall works?

Copy link
Contributor

Choose a reason for hiding this comment

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

for most users, cconvert followed by unsafe_convert was more detail than they would want to know, and that this gets the point across (i.e., that the passed values are converted to the appropriate type).

For any user, they should either not care about anything about conversion for ccall arguments at all, or if they do, they must not related it to convert. This change gets the wrong point across. I've recently seen people still thinking convert is the method to use for this and it's just wrong.

Bottom line is that I don't really mind if this is said in the footnote or not, but you must not mention convert.

@kmsquire
Copy link
Member Author

@Gnimuc I'll try to address your comments today.

BTW, I think we'd better mention more use cases for GC.@preserve in the doc.

I don't have enough grasp of this to add anything useful.
If you'd like to, why don't you either make a PR against my branch here (I'll preserve your commits), or just make a new PR (probably after this is merged).

* Formatting, wording updates
* Revery the creation of footnote 3.  Part of that note was put back
  into the text, and part was deleted, as it was pointed out that
  it was misleading.
* Remove some additional misleading comments regarding the use of Ptr
  and Ref.
@kmsquire
Copy link
Member Author

Okay, I've "addressed" @Gnimuc's comments mostly by deleting the offending section. If anyone has some better wording that could be put in instead, please speak up!

@Gnimuc @fredrikekre @yuyichao If you have time, can you take a glance through the document as it stands right now and let me know if there' s anything else that should be reworded or removed? Thanks.

doc/src/manual/calling-c-and-fortran-code.md Outdated Show resolved Hide resolved
doc/src/manual/calling-c-and-fortran-code.md Outdated Show resolved Hide resolved
doc/src/manual/calling-c-and-fortran-code.md Outdated Show resolved Hide resolved
doc/src/manual/calling-c-and-fortran-code.md Outdated Show resolved Hide resolved
doc/src/manual/calling-c-and-fortran-code.md Outdated Show resolved Hide resolved
doc/src/manual/calling-c-and-fortran-code.md Outdated Show resolved Hide resolved
doc/src/manual/calling-c-and-fortran-code.md Outdated Show resolved Hide resolved
doc/src/manual/calling-c-and-fortran-code.md Outdated Show resolved Hide resolved
doc/src/manual/calling-c-and-fortran-code.md Outdated Show resolved Hide resolved
doc/src/manual/calling-c-and-fortran-code.md Outdated Show resolved Hide resolved
@kmsquire
Copy link
Member Author

Updated according to the latest review. @vtjnash (or anyone else), would you like to take another quick scan?

The test failure is (probably) #31712.

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.

6 participants