-
-
Notifications
You must be signed in to change notification settings - Fork 34
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
Compatibility with LibCURL's new generator script #134
Conversation
Oh this commit uses a custom fork |
src/Curl/utils.jl
Outdated
@@ -33,7 +33,7 @@ function check(ex::Expr, lock::Bool) | |||
end | |||
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)
1 Those 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)
0x00000033 So, for example when you write The The fields in the |
src/Curl/utils.jl
Outdated
@@ -33,7 +33,7 @@ function check(ex::Expr, lock::Bool) | |||
end | |||
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:
Integer
in 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/SnVsaWEgaXMgZ3JlYXQh
returnscurl: (35) OpenSSL SSL_connect: Connection reset by peer in connection to httpbingo.julialang.org:443
without a proxy, for example)