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.

Sign up for GitHub

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

Jump to bottom

Use unboxed types for uint8_t and uint16_t #413

Merged
merged 10 commits into from
Aug 15, 2016
Merged

Use unboxed types for uint8_t and uint16_t #413

merged 10 commits into from
Aug 15, 2016

Conversation

DemiMarie
Copy link
Contributor

This PR changes the representation of uint8_t and uint16_t to be unboxed values. This should be a performance win.

@yallop
Copy link
Owner

yallop commented Jul 2, 2016

Thank you. I think this is a worthwhile change.

The tests are failing on OCaml 4.01.0 because of the [@@inline] annotations, which were introduced in OCaml 4.02.0. Could you please remove them?

@DemiMarie
Remove [@@inline] annotations
1fc2d9d 
[@@inline] is not supported by OCaml 4.01, so remove it.
@DemiMarie
Copy link
Contributor Author

@yallop I removed the [@@inline] annotations. This should not be a big deal: these tiny functions will almost certainly be inlined anyway.

yallop
yallop reviewed Jul 2, 2016
View reviewed changes
src/ctypes/unsigned.ml
let to_int64 : t -> int64 = fun x -> to_int x |> Int64.of_int [@@inline]
external of_string : string -> t = "ctypes_uint8_of_string" [@@inline]
let to_string : t -> string = string_of_int [@@inline]
let of_int64 : int64 -> t = fun x -> of_int (Int64.to_int)
Copy link
Owner

Choose a reason for hiding this comment

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

(Int64.to_int) ~> (Int64.to_int x)

@DemiMarie
Fix typo in src/ctypes/unsigned.ml
a4f8368 
This caused compilation to fail with a typing error.
@yallop
Copy link
Owner

yallop commented Jul 4, 2016 •
edited
Loading

The patch changes the behaviour around unrepresentable values in a couple of ways:

  • In the current implementation unrepresentable values are truncated on conversion to uint8 (etc.):

    # Unsigned.UInt8.(of_int 256);;
- : Unsigned.UInt8.t = <uint8 0>

    whereas with this PR they're rejected with an exception.

    # Unsigned.UInt8.of_int 256;;
Exception: Invalid_argument "argument out of range".

    Both behaviours are reasonable, but the truncation behaviour more closely matches the behaviour of OCaml's standard library:

    # Int32.of_int 2147483648;;
- : int32 = -2147483648l

  • In the current implementation operations on uint8 values always produce valid uint8 values:

    # Unsigned.UInt8.(add one (of_int 255));;
- : Unsigned.UInt8.t = <uint8 0>

    whereas with this PR they can produce invalid uint8 values:

    # Unsigned.UInt8.(add one (of_int 255));;
- : Unsigned.UInt8.t = <uint8 256>

Could you please switch the patch over to the existing behaviour?

@DemiMarie
Fix overflow of uint8 and uint16
2b193ae 
I had previously (and incompatibly) changed the overflow behavior
of uint8 and uint16 such that they failed to check for overflow
during arithmatic, and raised Invalid_argument when an overflow
occurred durint their creation (in of_int).  This patch fixes both
of those.
@DemiMarie
Copy link
Contributor Author

@yallop done. I just noticed that there are no tests for this behavior – should I add some?

@yallop
Copy link
Owner

yallop commented Jul 11, 2016

Thanks. Yes, some tests would be very welcome.

@DemiMarie
Add tests for unsigned coercions
e5ac383 
Add tests for coercions of unsigned integers
@DemiMarie
Copy link
Contributor Author

Tests added.

@yallop
Copy link
Owner

yallop commented Jul 25, 2016

Thanks for the tests. Could you please address the following minor cosmetic issues? Once they're fixed I'm happy to merge this PR.

  • in unsigned_stubs.c:
    uint8_t_custom_val and uint16_t_custom_val are unused, and should be removed.
  • in cstubs_c_language.ml and elsewhere (ctypes_unsigned_stubs.h etc.)
    ctypes_copy_uint8 should now be called Ctypes_val_uint8 (sim. ctypes_copy_uint16), since
    it no longer copies/allocates.
  • in ctypes_ptr.ml:
    Please revert the changes to of_nativeint
    (Rationale: I prefer type-checked functions to unchecked externals, and the existing implementation will certainly be inlined)
  • in unsigned.ml:
    Please remove the commented-out code (twice) (* if x <= max_int then x else invalid_arg "argument out of range" *)
  • in test_constants.ml:
    Please remove the commented-out code in the test_retrieve_constants function.

@DemiMarie
Respond to review by @yallop
2b93a8d 
Deletes commented-out code, fixes naming of ctypes_copy_uint16 and
ctypes_copy_uint8, and adds unit tests.
@DemiMarie
Copy link
Contributor Author

I replaced the commented-out Printf.printf calls with extra unit tests, since the Printf.printf calls were extremely helpful during debugging.

@yallop
Copy link
Owner

yallop commented Aug 15, 2016

Thank you!

@yallop yallop merged commit 675c35d into yallop:master Aug 15, 2016
@yallop yallop mentioned this pull request Aug 17, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DemiMarie @yallop