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

Always store OCaml representation as a struct #55

Merged
merged 2 commits into from
Oct 28, 2020

Conversation

dra27
Copy link
Contributor

@dra27 dra27 commented Oct 28, 2020

This is split off from #50 and will shortly be an alternative to it. This version always stores the number as a struct and reads it into a 128-bit integer.

@dra27
Copy link
Contributor Author

dra27 commented Oct 28, 2020

@rixed wrote:

Other than that, don't you think the high and low members of int128_ocaml struct should be swapped in the hope the compiler might optimize the shift away on those architectures where it's safe to do so?

To be honest, I just copied the struct from the incomplete emulation side, although it did mean in an earlier version of what's now in #50 I accidentally had one of the 128-bit copies big endian and one little endian which was, um, amusing 🤦‍♂️

@rgrinberg
Copy link
Collaborator

@rixed awaiting your review.

@rgrinberg
Copy link
Collaborator

a CHANGES entry would be welcome as well.

@rixed
Copy link
Contributor

rixed commented Oct 28, 2020

@rixed awaiting your review.

This looks good to me!

dra27 added 2 commits October 28, 2020 11:45
Pointers to 128-bit numbers must be 16-byte aligned which OCaml cannot
guarantee. Always store the number in a struct and read it into a 128-bit
integer.
@rgrinberg rgrinberg merged commit f14fe83 into andrenth:master Oct 28, 2020
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Oct 28, 2020
CHANGES:

## Fixes:

* Correct conversion from uint24 to other ints (andrenth/ocaml-stdint#39, @rixed)
* Fix conversion from all ints to uint24 and int24 (andrenth/ocaml-stdint#41, @rixed)
* Fix int24 failing to recover from casts (andrenth/ocaml-stdint#43, @rixed)
* Fix sign extensions (andrenth/ocaml-stdint#49, @rixed)
* `Long_val` returns `intnat`, previously `long` was used (andrenth/ocaml-stdint#53, @dra27)
* Reduce size of marshalled custom values on 4.08+ (andrenth/ocaml-stdint#54, @dra27)
* Store 128-bit ints as structs to prevent unaligned access (andrenth/ocaml-stdint#55, @dra27)

## New features:

* Add `of_substring` (andrenth/ocaml-stdint#49, @darlentar)
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Oct 28, 2020
CHANGES:

## Fixes:

* Correct conversion from uint24 to other ints (andrenth/ocaml-stdint#39, @rixed)
* Fix conversion from all ints to uint24 and int24 (andrenth/ocaml-stdint#41, @rixed)
* Fix int24 failing to recover from casts (andrenth/ocaml-stdint#43, @rixed)
* Fix sign extensions (andrenth/ocaml-stdint#49, @rixed)
* `Long_val` returns `intnat`, previously `long` was used (andrenth/ocaml-stdint#53, @dra27)
* Reduce size of marshalled custom values on 4.08+ (andrenth/ocaml-stdint#54, @dra27)
* Store 128-bit ints as structs to prevent unaligned access (andrenth/ocaml-stdint#55, @dra27)

## New features:

* Add `of_substring` (andrenth/ocaml-stdint#49, @darlentar)
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