Skip to content
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

int64 encoding is broken in the OCaml lib for negative numbers #63

Open
reebalazs opened this issue Nov 14, 2023 · 2 comments
Open

int64 encoding is broken in the OCaml lib for negative numbers #63

reebalazs opened this issue Nov 14, 2023 · 2 comments

Comments

@reebalazs
Copy link

Spice uses Caml_int64.bits_of_float / Caml_int64.float_of_bits but I've noticed that for negative numbers float_of_bits gives NaN. Therefore I have the suspicion that this library is broken. Consequently, spice is broken with regards to converting negative int64 values.

I've asked this on the rescript forums: https://forum.rescript-lang.org/t/int64-float-of-bits-gives-nan-for-a-negative-integer/4793/1

I don't know what's the best solution, for now I'm avoiding to convert a negative int64, but for the sake of completeness, this should also work in spice.

@mununki
Copy link
Member

mununki commented Nov 19, 2023

Hi, Sorry for late response.
Frankly, I didn't acknowledge this issue before. Thank you for openning issue. As I shortly dig into this issue, I reach to the consideration of what about the compiler has the bigint primitive such as int32, int64. Let me think about this further, and I'll come back later.

@reebalazs
Copy link
Author

reebalazs commented Nov 21, 2023

Hi, thanks! Let me know if I can help in any way! It would be great to fix this in a good way.

My current standpoint is:

  • The bits_of encoding would be better (storing as a number is more economic that storing in string) but it has to work for negatives too. It's unclear for me if the ocaml library is broken, how come noone has noticed this? Maybe this would also need a fix in the rescript compiler repo, then, but until then, spice could fix it on its own account.

  • Regardless of the previous point, it might be beneficial to support a to_string codec for int64s, as an alternate codec that is usable from spice if needed. In some cases you'd prefer a string codec...

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

No branches or pull requests

2 participants