Notes from the C Wrapper

[odow]

https://github.com/fico-xpress/XpressAPI.jl/blob/f20e039fea38b2cd964d99d3c2c3e681ff874fe2/src/C_wrapper/C_wrapper.jl#L203

Do import Libdl instead

https://github.com/fico-xpress/XpressAPI.jl/blob/f20e039fea38b2cd964d99d3c2c3e681ff874fe2/src/C_wrapper/C_wrapper.jl#L204

Prefer import SparseArrays so that all subsequent usages require SparseArrays. as the prefix. It makes things easier to maintain in the future, and it avoids SparseArrays releasing a new public symbol that conflicts with one of yours

https://github.com/fico-xpress/XpressAPI.jl/blob/f20e039fea38b2cd964d99d3c2c3e681ff874fe2/src/C_wrapper/C_wrapper.jl#L213

Remove the = nothing. This creates a new method Base.showerror(io::IO) = print(io, nothing.message) which is both type piracy (creating a method with types that you do not own) and it is also broken.

https://github.com/fico-xpress/XpressAPI.jl/blob/f20e039fea38b2cd964d99d3c2c3e681ff874fe2/src/C_wrapper/C_wrapper.jl#L218-L228

Reconsider all of these. You shouldn't need a global Functions dictionary.

https://github.com/fico-xpress/XpressAPI.jl/blob/f20e039fea38b2cd964d99d3c2c3e681ff874fe2/src/C_wrapper/C_wrapper.jl#L242-L268

Reconsider this. See how Xpress.jl searches for the library at Pkg.build time https://github.com/jump-dev/Xpress.jl/blob/master/deps/build.jl or loads it on __init__: https://github.com/jump-dev/Xpress.jl/blob/3f08ab203011da3375c1dd1af72cbb16c40b00bf/src/Xpress.jl#L22

https://github.com/fico-xpress/XpressAPI.jl/blob/f20e039fea38b2cd964d99d3c2c3e681ff874fe2/src/C_wrapper/C_wrapper.jl#L280-L284

This can be

p_major, p_minor, p_build = Ref{Cint}(0), Ref{Cint}(0), Ref{Cint}(0)
ccall((:XPRSgetversionnumbers, libxprs), Cint, (Ref{Cint}, Ref{Cint}, Ref{Cint}), p_major, p_minor, p_build)
# or
p_major, p_minor, p_build = Ref{Cint}(0), Ref{Cint}(0), Ref{Cint}(0)
@ccall libxprs.XPRSgetversionnumbers(p_major::Ref{Cint}, p_minor::Ref{Cint}, p_build::Ref{Cint})::Cint

https://github.com/fico-xpress/XpressAPI.jl/blob/f20e039fea38b2cd964d99d3c2c3e681ff874fe2/src/C_wrapper/C_wrapper.jl#L376

You don't need these explicit export because you have the export loop in XpressAPI.jl.

https://github.com/fico-xpress/XpressAPI.jl/blob/f20e039fea38b2cd964d99d3c2c3e681ff874fe2/src/C_wrapper/C_wrapper.jl#L705-L715

See the equivalent code in Xpress.jl: https://github.com/jump-dev/Xpress.jl/blob/3f08ab203011da3375c1dd1af72cbb16c40b00bf/src/Xpress.jl#L234-L239. Note that you need GC.@preserve to retain the buffer. Also, your retcode is unchecked (although I just noticed that ours is too), and the try-catch is unnecessary because I don't think it's possible for that to go wrong. Something similar appears in many other places.

https://github.com/fico-xpress/XpressAPI.jl/blob/f20e039fea38b2cd964d99d3c2c3e681ff874fe2/src/C_wrapper/C_wrapper.jl#L772

Statements at the top level of a module don't need global in front of them. They are by default.

https://github.com/fico-xpress/XpressAPI.jl/blob/f20e039fea38b2cd964d99d3c2c3e681ff874fe2/src/C_wrapper/C_wrapper.jl#L11954

It's very tempting to play laissez-faire with types in Julia. But we typically find that it's better to start restrictive and widen only if there is demand. Instead of AbstractString in places, consider using String.

The same happens here:

https://github.com/fico-xpress/XpressAPI.jl/blob/f20e039fea38b2cd964d99d3c2c3e681ff874fe2/src/C_wrapper/C_wrapper.jl#L11999-L12001

You could make XPRSsetcheckedmode(checkedmode::Int32)::Nothing and force the complexity onto the caller. Rather than casting from an checkedmode::Any input into a Int32(checkedmode). (That might fail.)

https://github.com/fico-xpress/XpressAPI.jl/blob/f20e039fea38b2cd964d99d3c2c3e681ff874fe2/src/C_wrapper/C_wrapper.jl#L12013

The docstring is incorrect. We typically expect )::checkedmodeto be the type, not the name of the return value. SoXPRSgetcheckedmode()::Int32`.

https://github.com/fico-xpress/XpressAPI.jl/blob/f20e039fea38b2cd964d99d3c2c3e681ff874fe2/src/C_wrapper/C_wrapper.jl#L12033

Here and throughout, consider explicit return statements. They make the code easier to read.

https://github.com/fico-xpress/XpressAPI.jl/blob/f20e039fea38b2cd964d99d3c2c3e681ff874fe2/src/C_wrapper/C_wrapper.jl#L12080

This is unsafe behaviour. You need

GC.@preserve version_buffer
    version = unsafe_string(pointer(version_buffer))
end

https://github.com/fico-xpress/XpressAPI.jl/blob/f20e039fea38b2cd964d99d3c2c3e681ff874fe2/src/C_wrapper/C_wrapper.jl#L12348

Here's a question that needs a much longer discussion amongst yourselves. How do you translate Julia types to C types and vice versa? So in this one, filename can be nothing, which gets converted to C_NULL internally. The alternative would be to require the user to pass C_NULL directly. The docstring is also missing an explanation of what nothing does.

https://github.com/fico-xpress/XpressAPI.jl/blob/f20e039fea38b2cd964d99d3c2c3e681ff874fe2/src/C_wrapper/C_wrapper.jl#L12447-L12466

Same question about types here. Prefer concrete types because:

https://github.com/fico-xpress/XpressAPI.jl/blob/f20e039fea38b2cd964d99d3c2c3e681ff874fe2/src/C_wrapper/C_wrapper.jl#L12487-L12493

it might be that rhs::AbstractVector{Number} but no method is defined for Base.convert(::Type{Vector{Float64}}, rhs). Similarly, if you leave c_rhs as a AbstractVector{Float64} there might not be a method to convert it to a Ptr{Float64}. As one example:

julia> x = 1.0:4.0
1.0:1.0:4.0

julia> x isa AbstractVector{Float64}
true

julia> y = Base.cconvert(Ptr{Cdouble}, x)
1.0:1.0:4.0

julia> z = Base.unsafe_convert(Ptr{Cdouble}, y)
ERROR: conversion to pointer not defined for StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64}
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:44
 [2] unsafe_convert(::Type{Ptr{…}}, a::StepRangeLen{Float64, Base.TwicePrecision{…}, Base.TwicePrecision{…}, Int64})
   @ Base ./pointer.jl:66
 [3] top-level scope
   @ REPL[13]:1
Some type information was truncated. Use `show(err)` to see complete types.

The "flexibility" of types in Julia is both a blessing and a curse. It makes it easy to write high level code, but it can make low-level library code hard to write in a generic fashion.

https://github.com/fico-xpress/XpressAPI.jl/blob/f20e039fea38b2cd964d99d3c2c3e681ff874fe2/src/C_wrapper/C_wrapper.jl#L29841-L29853

This appears to be the only function that is 1-based indexing instead of 0-based?

[AdamFDC]

Thanks a lot for this detailed feadback, most of this is now addressed in v47.1.4