ocaml-ipaddr

ocaml-ipaddr copied to clipboard

We currently have this:

  (** {3 Bytestring conversion} *)

  (** [of_bytes_exn ipv4_octets] is the address represented
      by [ipv4_octets]. Raises [Parse_error] if [ipv4_octets] is not a
      valid representation of an IPv4 address. *)
  val of_bytes_exn : string -> t

  (** Same as [of_bytes_exn] but returns an option type instead of raising
      an exception. *)
  val of_bytes : string -> t option

  (** Same as [of_bytes_exn] but take an extra paramenter, the offset into
      the bytes for reading. *)
  val of_bytes_raw : string -> int -> t

From reading the docstring it was unclear to me that

if [ipv4_octets] is not a valid representation of an IPv4 address

actually means (which AFAICT is the only failure mode?):

if [ipv4_octets] is not exactly 4 bytes long.

It is also misleading that of_bytes_raw suggests that it is the same as of_bytes_exn; as evidenced below, of_bytes_raw does not impose an upper bound on the string length:

  let of_bytes_raw bs o =
    make
      (Char.code bs.[0 + o])
      (Char.code bs.[1 + o])
      (Char.code bs.[2 + o])
      (Char.code bs.[3 + o])

  let of_bytes_exn bs =
    let len = String.length bs in
    if len > 4 then raise (too_much bs);
    if len < 4 then raise (need_more bs);
    of_bytes_raw bs 0

Not overly important I guess, but this tripped me during a code review.

thanks for raising this issue. the API looks too complex for me -- is anyone using of_bytes_raw? AFAICT (using github.com search facilities), vbmithr/ocaml-llsqlite vbmithr/ocaml-llnet and cfcs/ocaml-socks are using this function. I'd be in favour of deprecating of_bytes_raw (and of_string_raw), and providing only of_bytes and of_bytes_exn -- this would impose the burden on the API user to String.sub their data to the correct offset.

and as mentioned in #35 #36 #37 we should redesign (and provide of_cstruct / to_cstruct) this API. any takers? (hint: #69 should as well be addressed at that time)

I think of_bytes_raw is nicer than of_bytes_exn, it make the calling code much prettier since you don't need to spray String.(sub s 2 4) everywhere.

EDIT: To be clear I was only complaining about the docstrings :)

I'd prefer to keep of_bytes_raw and fix the docstrings :)

has been fixed by @avsm finally in #89 AFAICT, please reopen (or PR) if there's anything left to do. I'm fine with keeping the _raw functions, sorry for the noise.

Ipaddr.V6.of_octets{,_exn} suffers from the same issue.