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

feat: add small signed integers #49

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

zbaylin
Copy link

@zbaylin zbaylin commented Oct 8, 2023

This PR adds 8 and 16 bit signed integer support. I attempted to mirror as much of the existing code as possible, but there still might be room for improvement -- it's been awhile since I've written much C.

I'll add some comments on the code itself to explain some of the rationale/outstanding questions.

#define OCAML_INTEGERS_INTERNAL 1
#include "ocaml_integers.h"

CAMLprim value integers_int_size(value unit)
Copy link
Author

Choose a reason for hiding this comment

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

Sys.int_size was added in 4.03, so I copied this small function from the compiler distribution.

Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer to update the minimum requirement to OCaml 4.03 and call the function from the distribution

base = 8; pos += 2; break; \
case 'b': case 'B': \
base = 2; pos += 2; break; \
case 'u': case 'U': /* Unsigned prefix. No-op for unsigned types */ \
Copy link
Author

Choose a reason for hiding this comment

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

I left this comment in place because I'm still not 100% sure what the behavior here should be after reading the comments in Stdlib.int_of_string -- I think it's possible this case is already taken care of here since we're parsing directly into an integer of correct size rather than an intnat?

let shift_left : t -> int -> t = fun x y -> fix (x lsl y)
let shift_right : t -> int -> t = ( asr )
let shift_right_logical : t -> int -> t = fun x y ->
((x lsl (int_size - bits)) lsr y) asr (int_size - bits)
Copy link
Author

Choose a reason for hiding this comment

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

This is done to handle the case of preservation when the RHS is 0 -- it's a bit ugly so if there's a better way to do it I'm all ears.

Comment on lines +90 to +92
let of_string = of_string
let to_string = to_string
let to_hexstring = to_hexstring
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
let of_string = of_string
let to_string = to_string
let to_hexstring = to_hexstring
let of_string = S.of_string
let to_string = S.to_string
let to_hexstring = S.to_hexstring

(for consistency with min_int and max_int above).


module MakeSmall(S : Small) =
struct
open S
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
open S

(see suggestion to use qualified names below)

return caml_copy_string(buf); \
} \
\
/* to_hexstring : t -> string */ \
Copy link
Owner

Choose a reason for hiding this comment

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

The specification of to_hexstring (which should really be more clearly documented in the existing code) is

hex pretty-printing (to mimic "%x")

(see #33 (comment)).

which means, e.g.:

# Printf.sprintf "%x" (-15);;
- : string = "7ffffffffffffff1"

@@ -0,0 +1,81 @@
let () = print_endline "Small int operations test: ?"
Copy link
Owner

Choose a reason for hiding this comment

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

It'd be good to have some tests for parsing and printing

@brendanzab
Copy link
Contributor

brendanzab commented Nov 11, 2024

Any word on getting this merged and published? It would be really handy!

Edit: Created a PR addressing the review comments at #51

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