Skip to content

Updated repository to Julia 0.7+ and 1.1+ #8

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

Closed
wants to merge 2 commits into from

Conversation

chakravala
Copy link
Contributor

@chakravala chakravala commented Feb 14, 2019

This pull-request updates the CBOR repository to be compatible with Julia 0.7+ and 1.1+

This also resolves the issue discussed in #7 with @stevengj

Unfortunately, the tests are not all passing yet. Some more tweaks need to be made, but I am not familiar with what is causing the problem.

Test Failed at /home/flow/.julia/dev/CBOR/test/runtests.jl:83
  Expression: isequal(data, decode(bytes))
   Evaluated: isequal(6.103515625e-5, 4.683311023e-315)
ERROR: LoadError: There was an error during testing
in expression starting at /home/flow/.julia/dev/CBOR/test/runtests.jl:81
ERROR: Package CBOR errored during testing
Stacktrace:

Could you please review this pull-request and see if you can fix the rest of the testing?

elseif addntl_info == ADDNTL_INFO_FLOAT16
error("Decoding 16-bit floats isn't supported.")
end

bytes_consumed += float_byte_len
const data = hex2num(bytes2hex(
data = reinterpret(Float64,parse(UInt64,bytes2hex(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need to use bytes2hex or parse at all here. For example, you could just do:

reinterpret(Float64, ntoh(reinterpret(UInt64, bytes[(start_idx + 1):(start_idx + float_byte_len)])))

This could be made even more efficient (if you care) by eliminating the creation and reinterpretation of a array for the bytes slice:

reinterpret(Float64, UInt64(bytes[start_idx + 1]) << 56 +
                     UInt64(bytes[start_idx + 2]) << 48 +
                     UInt64(bytes[start_idx + 3]) << 40 +
                     UInt64(bytes[start_idx + 4]) << 32 +
                     UInt64(bytes[start_idx + 5]) << 24 +
                     UInt64(bytes[start_idx + 6]) << 16 +
                     UInt64(bytes[start_idx + 7]) << 8 +
                     UInt64(bytes[start_idx + 8]))

Copy link
Member

@stevengj stevengj Feb 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if instead of working with bytes arrays, you read directly from an IO stream (or an IOBuffer), you could just do:

ntoh(read(io, Float64))

(where the ntoh is to fixe the byte order).

else
const float_bytes = hex2bytes(num2hex(float))
float_bytes = hex2bytes(string(reinterpret(Unsigned,float),base=16,pad=sizeof(float)*2))
Copy link
Member

@stevengj stevengj Feb 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If instead of using an array of bytes, you used an IO stream (or an IOBuffer), you could just write(io, hton(float)).

By converting the value to a string, then parsing into an array of bytes, and then pushing the array of bytes to another array, you are probably at least an order of magnitude slower, as well as requiring much more code.

@chakravala
Copy link
Contributor Author

Thanks for your comments @stevengj , I think it would be best if @saurvs could take your advice and implement it, since all I'm trying to do is to get the tests working reliably first.

Is the numerical error in the tests related to the hex2bytes conversions?

@SimonDanisch
Copy link
Member

Oh woops, why do I never look into PRs when upgrading a package...
Well, my PR passes tests: #9

@chakravala
Copy link
Contributor Author

Thanks for making progress on it, was hoping somebody would keep this package in order because I wanted to use it for one of my upcoming projects.

How is it coming along at the moment? I see you have not merged anything yet

@SimonDanisch
Copy link
Member

#10 took this over

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants