I strongly support adding these functions to the stdlib.

• I think adding little endian and big endian variants would be quite useful : a very common use-case for these functions is interfacing with binary protocols and/or formats in which case one always needs to use a specific endianness. A suffix _le and _be could be used. Adding the three versions (little endian, big endian, and host endian) may be optimal but a bit excessive.
• For the case of bigstrings (1-dimensional bigarrays), I think that adding a dedicated Bigstring module makes sense, containing roughly the union of operations on bytes and Bigarray. See also MPR#7772.
• For naming, is the suffix _bin really necessary? What about introducing a signature

module type S = sig
  type t
  val get_int16: t -> int
  val get_int32: t -> int32
  val get_int64: t -> int64
  val set_int16: t -> int -> unit
  val set_int32: t -> int32 -> unit
  val set_int64: t -> int64 -> unit
  (* etc *)
end

and define modules Bytes.BE : S with t := bytes, Bytes.LE : S with t := bytes, same for Buffer (with t := Buffer.t, etc), and so on. Unfortunately, this signature does not work for the case of channels (where the type t is different for get and set functions (maybe splitting the signature in an "input" part and an "output" part would be better to also handle the case of string).

• I tend to agree that exposing versions with explicit endianness would be good, but I think it's good to keep the version with the native endianness of the machine (which is useful e.g. for inter-process communication on the same machine, or to interact with buffers containing native "C" data). The number of functions is a bit worrying, though, between int16/int32/int64/float32/float64, le/be/native, string/bytes/bigstring/buffer(/channel), possibly safe/unsafe.

• An alternative could be to expose fewer functions, taking arguments, and hope/check that the inliner will be good enough to eliminate the overhead. E.g.:

type _ number_ty = Int16: int number_ty | Int32: int32 number_ty | ...
val Buffer.add_number: ?endian:[`LE|`BE](*default:Native*) -> Buffer.t -> 'a number_ty -> 'a -> unit  

• About the _bin suffix: I had in mind Stdlib.print_int and co, working with the ASCII decimal representation. But perhaps we don't care.

• I forgot about input_binary_int/output_binary_int, which we already have: big-endian, 4 bytes, but working with int, not int32.

Yes another approach would be to expose only functions using the native endianness, plus:

 val Int32.Endian.little: int32 -> int32
   (** Modify the value, if needed, so that functions such as Bytes.set_int32 produce a little-endian
        representation of the integer, independently of the native encoding of the host machine. Similarly,
        this function can be applied to the result of functions such as Bytes.get_int32 when reading
        a 32-bit integer in little-endian. *)

 val Int32.Endian.big: int32 -> int32
    (** .... *)

(and same for 16-bit and 64-bit)

This looks like a very useful addition.

Even if unsafe versions are not exposed, I think it would make
sense to avoid some bound checks in Buffer.add_xyz (they
are avoided in Buffer.add_char by using Bytes.unsafe_set
rather than Bytes.set).

(I would also consider adding add_int8/get_int8/set_int8.
They would of course be mere aliases to existing functions,
but would allow one to write more uniform code when working
on file formats or network protocols.)

In my experience what you need most of the time is the explicit endianness (network, codecs, etc.) and it's less error prone if the native endianness has as an explicit suffix; otherwise you tend to use it without thinking about the issue (or it will be the first one to auto-complete...) and you likely end up with a bug.

@alainfrisch I think your last suggestion would make things more confusing than enlighting. Personally I don't mind the number of functions. I think this should be simply:

{add,get,set}_int16_{ne,be,le} 
{add,get,set}_int32_{ne,be,le}
{add,get,set}_int64_{ne,be,le}

I don't think it's worth adding unsafe versions, people willing to go into unsafe territory can play the primitive game. I don't think it's worth adding the float versions this is an {Int64,Int32}.{bits_of_float,float_of_bits} away.

I don't think it's worth adding the float versions this is an {Int64,Int32}.{bits_of_float,float_of_bits} away.

I was concerned that this would cause extra boxing of the intermediate int64 if the function (e.g. Buffer.add_int64) is not inlined. To be checked.

Ok, it's seem fine to let the user do that float->int64 mapping (checked only with the "Closure" inliner, but I'm sure flambda will be ok), with default inlining settings.

I also check that the check for Sys.big_endian is eliminated in e.g. Buffer.add_int64_le, and that the performance is the same as Buffer.add_int64_ne (on a little-endian system).

For the naming: are _be, _le, _ne good enough?

Concerning the scope of this PR:

• I suggest to keep the idea of providing channel operations for later.
• Bigstring, which would be a whole new module, would deserve its own PR.
• Unsafe versions are indeed not needed (the impact on performance is rather small anyway).
• Perhaps adding get_int* functions to String makes sense, to keep it in sync with Bytes, even though most people would probably use bytes anyway for parsing binary data, I guess.

One more point: it could be useful to provide both get_int16 (sign extension) and get_uint16 (always returning positive integers).

Agreed on all points. I think it would be good to add the functions in String though.

One more point: it could be useful to provide both get_int16 (sign extension) and get_uint16 (always returning positive integers).

Yes ! FWIW I have this in an unpublished code of similar functionality. It's the kind of bit fiddling things you prefer to be handled by the library.

Two comments:

1. Why not include 8 bit quantities as well? Yes, the function to do so is trivial, but it will make some protocol handling code look more uniform.
2. About endianness, I'm always reminded of Rob Pike's insight that code that knows about endianness (other than compilers) is generally badly written. Here's his blog post on the topic. The code is all written in C but it's simple enough not to matter. https://commandcenter.blogspot.com/2012/04/byte-order-fallacy.html

About endianness, I'm always reminded of Rob Pike's insight that code that knows about endianness (other than compilers) is generally badly written.

I don't understand what this is supposed to bring to the discussion. Would you care to expand maybe ?

• Are we ok with 21 new functions just in Bytes (12 get, 9 set)? Should we organize them in submodules (per endianness) as suggested by @nojb? (I generally prefer having explicit signatures.)
• Assuming there is no runtime overhead, would it be good to pass the endianness choice (ne/le/be) as an argument, in order to divide the number of functions by 3?
• What about adding int8/uint8 variants?
• I believe that using Sys.int_size instead of Sys.word_size (as done in ocplib-endian) is better for JS backends, but that would need to be checked.

For comparison purposes, I should point out that I have implemented logic for this purpose in my Orsetto project. Start here for details if you're interested.

Accordingly, I have no strong opinion on how this pull request might be evaluated. My unsolicited opinions on the various questions discussed above summarized:

• unsafe interfaces are useful in the case where a higher-level interface is ensuring that data format invariants are respected.
• byte-order should not by inferred from the system architecture when formatting data for interchange, i.e. it should always be exposed in the programming interface without a default.
• formatting IEEE-754 floating point for interchange should provide support for binary16, binary32 and binary64 formats.

• I've tried with

val Buffer.add_int64: [`LE|`BE|`NE] -> t -> int64 -> unit

let[@ocaml.inline] add_int64 e b x =
  let new_position = b.position + 8 in
  if new_position > b.length then resize b 8;
  unsafe_set_int64 b.buffer b.position
    (match e, Sys.big_endian with
     | `LE, true | `BE, false -> swap64 x
     | _ -> x);
  b.position <- new_position

and thanks to the inline annotation, this gives the same performance than ad hoc functions when endianness is known statically.

• @jhwoodyatt : do you mean that we should not expose "native endianness" functions, and let the user rely on Sys.big_endian to choose between little/big endian variants if he really wants to use the native endianness (e.g. to interact with C datastructures)? This indeed makes it more explicit which parts of the code depends on the local native endianness.

and thanks to the inline annotation, this gives the same performance than ad hoc functions when endianness is known statically.

I think having all the function is in general more predictable. Except for guaranteed statically known checks done at the toplevel (like Sys.big_endian) I'm a little bit wary on relying too much on the compiler optimizations to get the baseline performance. Suppose I want to make something generic on endianness, with the current proposal I will abstract over the functions. With your last proposal I might abstract over [LE|NE|BE] which (I guess) might give poor performance if I'm not careful.

Personally I don't have a problem with all the additions (uint8 and int8 is something is also something I have in that unpublished code if you want to add them) and I personally don't see a huge benefit of gathering those in submodules. Having everything flat, including native endianness, is nice to write and obvious to read in my opinion.

@alainfrisch no, I mean that you should provide three variations and require the choice made explicitly in the programming interface:

• Little-endian byte order
• Big-endian byte order (or so-called "network byte order")
• System architecture byte order

That's what I did, and I think it was the right choice. Another thing I did was [basically] this:

type t = [ `BE | `LE | `SE ]
val create: [< t ] -> (module Endian)

Usually, the byte order is known statically. I think if dynamic byte order is really on the menu, then it's best to use a function that selects the right module.

@alainfrisch any progress on that one ? I would like to rely on this for further stdlib improvements.

Remaining points:

• Write the doc for all functions. I'd like to get feedback on the existing text before copying them furiously to all other functions
• Port functions from Bytes to String (once the exact design/doc for Bytes is stable)
• Add tests

Write the doc for all functions. I'd like to get feedback on the existing text before copying them furiously to all other functions

Reviewed, see my comments. I think we should keep the doc strings simple using the patterns I mentioned.

Just one minor pending point about the use of Char.unsafe_chr. Waiting for @dbuenzli feedback on that one.

Thanks for your help with the design and documentation!