-
-
Notifications
You must be signed in to change notification settings - Fork 38
Compatibility with LibCURL's new generator script #134
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
Conversation
|
Oh this commit uses a custom fork |
src/Curl/utils.jl
Outdated
| quote | ||
| r = $ex | ||
| iszero(r) || @async @error($prefix * string(r)) | ||
| iszero(Integer(r)) || @async @error($prefix * string(r)) |
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.
This seems incorrect. What's the intention here? Integer is an abstract type, not a concrete one. This won't change the type of any integer value, it will only affect floating-point values, and even if one somehow got a float here, iszero would still work.
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.
The appropriate fix is to add another definition: Base.zero(::CEnum.Cenum{T}) where {T<:Integer} = zero(T).
|
Sorry I did not know the design of Downloads.jl, I thought the code was only intended to work with LibCURL.jl. If so, it should directly map to LibCURL's constants instead of some integer values. For example, the |
|
Thanks for the effort here. A few general comments. julia> Integer(1)
1
julia> Integer(0x1)
0x01
julia> Integer(1.0)
1Those LibCURL constants are all already integers so doing julia> LibCURL.CURLE_PEER_FAILED_VERIFICATION
0x00000033
julia> typeof(ans)
UInt32
julia> Integer(LibCURL.CURLE_PEER_FAILED_VERIFICATION)
0x00000033So, for example when you write The The fields in the |
src/Curl/utils.jl
Outdated
| quote | ||
| r = $ex | ||
| iszero(r) || @async @error($prefix * string(r)) | ||
| iszero(Integer(r)) || @async @error($prefix * string(r)) |
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.
The appropriate fix is to add another definition: Base.zero(::CEnum.Cenum{T}) where {T<:Integer} = zero(T).
|
@melonedo would you like to reopen this and adapt to the comments? |
193060f to
667162d
Compare
|
Now I included conversion to |
|
Closed in favor of JuliaWeb/LibCURL.jl#105 |
Ref: LibCURL.jl#102
I have tested this new wrapper, and Downloads.jl does not work with the new wrapper, mainly because:
Integerin Downloads.jlthis patch adds compatibility to the new wrapper. Most tests are passed, except the ones that can not be run on my network (
curl https://httpbingo.julialang.org/base64/SnVsaWEgaXMgZ3JlYXQhreturnscurl: (35) OpenSSL SSL_connect: Connection reset by peer in connection to httpbingo.julialang.org:443without a proxy, for example)