Add unsafe_convert method for CbcProblem. Resolve #100. #101
Conversation
|
Thanks for the fix. I'd like a decisive answer about whether function arguments need to be preserved because if that's the case we need to make a lot of changes to wrappers. I wasn't able to follow the post you linked at #100 (comment). |
|
|
src/CbcCInterface.jl
Outdated
| end | ||
|
|
||
| function writeMps(prob::CbcModel, filename::String) | ||
| check_problem(prob) | ||
| @assert isascii(filename) | ||
| @cbc_ccall writeMps Cint (Ptr{Cvoid}, Ptr{UInt8}) prob.p filename | ||
| GC.@preserve prob filename begin |
There was a problem hiding this comment.
filename is a direct argument, no preserve needed?
There was a problem hiding this comment.
So this one (and the one you commented on below) I'm not sure of. I think it could currently still be an issue if writeMps happens to get inlined, and given Stefan's comment, there don't seem to be future guarantees regarding input arguments anyway. Again, I'm not sure of the cost of GC.@preserve, but I'd assume it'd be negligible compared to the ccall.
There was a problem hiding this comment.
The documentation makes a strong statement about ccall arguments: https://docs.julialang.org/en/v1/manual/calling-c-and-fortran-code/#Garbage-Collection-Safety-1
ccall automatically arranges that all of its arguments will be preserved from garbage collection until the call returns.
I interpret that to mean that if this behavior changes it can only do so In a new major Julia release. I agree that the extra preserve is unlikely to have a performance impact. I'm nit picking so that we apply consistent rules and don't confuse someone who looks at this code and wonders why we preserved this particular argument when it's not needed.
There was a problem hiding this comment.
Would it be cleaner to define a convert(::Type{Ptr{Cvoid}},::CbcModel) method and just pass prob to ccall instead? See https://docs.julialang.org/en/v1/manual/calling-c-and-fortran-code/#Auto-conversion:-1
There was a problem hiding this comment.
I'm fully onboard with the nitpicking here since this has ramifications for many other packages. I think @andreasnoack's idea is a good one.
There was a problem hiding this comment.
Actually, according to https://docs.julialang.org/en/v1/base/c/#Base.cconvert,
Neither
convertnorcconvertshould take a Julia object and turn it into aPtr
So I think this should be a Base.unsafe_convert method.
tkoolen
changed the title
|
Thanks for the fix! |
No description provided.