-
Notifications
You must be signed in to change notification settings - Fork 31
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 wrapper of xprs.h #257
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #257 +/- ##
==========================================
+ Coverage 67.91% 67.93% +0.01%
==========================================
Files 6 6
Lines 3301 3303 +2
==========================================
+ Hits 2242 2244 +2
Misses 1059 1059 ☔ View full report in Codecov by Sentry. |
# For backwards compatibility, older versions of Xpress.jl used Cint | ||
# instead of Clong. | ||
# TODO(odow): this is wrong. Fix me in a separate PR. | ||
contents = replace(contents, "Clong" => "Cint") |
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.
So I found one super weird thing here. The xprs.jl
uses Cint
in places where it should use Clong
. They are equivalent on windows, but not on Mac or Linux. I don't know if this causes any issues because we don't call the 64(
API.
# with older versions of Xpress.jl | ||
# | ||
# We need to replace char* with Ptr{UInt8} and char[] with | ||
# Cstring. |
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.
Okay. I've hacked around to get the right Cstring
and Ptr{UInt8}
in the right places. Focusing on minimal changes. We can reconsider in a different PR.
I think this really highlights that we should have some way of testing v8 |
setvals, | ||
) | ||
if get_version() >= v"41.01" | ||
@checked Lib.XPRSgetmipentities( |
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.
These bits were nasty:
Lines 2366 to 2394 in ceb9df8
function _getversion() | |
buffer = Array{Cchar}(undef, 8 * 24) | |
buffer_p = pointer(buffer) | |
GC.@preserve buffer begin | |
out = Cstring(buffer_p) | |
r = Lib.XPRSgetversion(out) | |
if r != 0 | |
throw(XpressError(r, "Unable to invoke XPRSgetversion")) | |
end | |
version = unsafe_string(out)::String | |
end | |
return VersionNumber(parse.(Int, split(version, "."))...) | |
end | |
function XPRSgetglobal(prob, p_nentities, p_nsets, coltype, colind, limit, settype, start, setcols, refval) | |
if _getversion() >= v"41.0.0" | |
return ccall((:XPRSgetmipentities, libxprs), Cint, (XPRSprob, Ptr{Cint}, Ptr{Cint}, Ptr{UInt8}, Ptr{Cint}, Ptr{Cdouble}, Ptr{UInt8}, Ptr{Cint}, Ptr{Cint}, Ptr{Cdouble}), prob, p_nentities, p_nsets, coltype, colind, limit, settype, start, setcols, refval) | |
else | |
return ccall((:XPRSgetglobal, libxprs), Cint, (XPRSprob, Ptr{Cint}, Ptr{Cint}, Ptr{UInt8}, Ptr{Cint}, Ptr{Cdouble}, Ptr{UInt8}, Ptr{Cint}, Ptr{Cint}, Ptr{Cdouble}), prob, p_nentities, p_nsets, coltype, colind, limit, settype, start, setcols, refval) | |
end | |
end | |
function XPRSgetglobal64(prob, p_nentities, p_nsets, coltype, colind, limit, settype, start, setcols, refval) | |
if _getversion() >= v"41.0.0" | |
return ccall((:XPRSgetmipentities64, libxprs), Cint, (XPRSprob, Ptr{Cint}, Ptr{Cint}, Ptr{UInt8}, Ptr{Cint}, Ptr{Cdouble}, Ptr{UInt8}, Ptr{Cint}, Ptr{Cint}, Ptr{Cdouble}), prob, p_nentities, p_nsets, coltype, colind, limit, settype, start, setcols, refval) | |
else | |
return ccall((:XPRSgetglobal64, libxprs), Cint, (XPRSprob, Ptr{Cint}, Ptr{Cint}, Ptr{UInt8}, Ptr{Cint}, Ptr{Cdouble}, Ptr{UInt8}, Ptr{Cint}, Ptr{Cint}, Ptr{Cdouble}), prob, p_nentities, p_nsets, coltype, colind, limit, settype, start, setcols, refval) | |
end | |
end |
Okay. I'm feeling a lot better about this now that we're testing on v8 and v9. |
Merging because I want to open a couple of follow-ups. |
I need to figure out a good way of handling the variety of Xpress versions that people might use.
I have headers for v33.01.12 and v42.01.05. Ideally, the Xpress.jl should use only functions defined in v33, but it should provide constants etc for v42.