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

Clarify bytestring conversion docstrings #81

Closed
cfcs opened this issue Jan 5, 2019 · 5 comments · Fixed by ocaml/opam-repository#25919
Closed

Clarify bytestring conversion docstrings #81

cfcs opened this issue Jan 5, 2019 · 5 comments · Fixed by ocaml/opam-repository#25919

Comments

@cfcs
Copy link

cfcs commented Jan 5, 2019

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.
@hannesm
Copy link
Member

hannesm commented Jan 5, 2019 •
edited

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)

@cfcs
Copy link
Author

cfcs commented Jan 5, 2019 •
edited

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 :)

@avsm
Copy link
Member

avsm commented Jan 6, 2019

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

avsm added a commit to avsm/ocaml-ipaddr that referenced this issue Jul 9, 2019
@avsm
rename the bytes functions to refer to octets instead
358ead3 
This clears up some confusion with the new ocaml `bytes` type.

Fixes mirage#35 mirage#81
@hannesm
Copy link
Member

hannesm commented Jul 10, 2019

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.

@hannesm hannesm closed this as completed Jul 10, 2019
@reynir
Copy link
Member

reynir commented Oct 1, 2023

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

@reynir reynir reopened this Oct 1, 2023
hannesm added a commit to hannesm/ocaml-ipaddr that referenced this issue May 22, 2024
@hannesm
adapt the V6 of_string* docstrings from the V4 versions, fixes mirage#81
c9b461d
hannesm added a commit to hannesm/ocaml-ipaddr that referenced this issue May 22, 2024
@hannesm
adapt the V6 of_string* docstrings from the V4 versions, fixes mirage#81
43addbf
hannesm added a commit that referenced this issue May 22, 2024
@hannesm
Merge pull request #123 from hannesm/fix-docstring
93085cf 
adapt the V6 of_string* docstrings from the V4 versions, fixes #81
hannesm added a commit to hannesm/opam-repository that referenced this issue May 22, 2024
@hannesm
[new release] ipaddr (6 packages) (5.6.0)
fd70f08 
CHANGES:

* Add host and subnet Seq.t iterators for Ipaddr.V4.prefix, Ipaddr.V6.Prefix
  and Ipaddr.Prefix (mirage/ocaml-ipaddr#107 @verbosemode @dinosaure)
* Ipaddr.V4.compare: use Int32.unsigned_compare, as provided since OCaml 4.08
  (mirage/ocaml-ipaddr#118 @reynir)
* Ipaddr.V6.t is a string, not a byte vector anymore (so it is immutable)
  (mirage/ocaml-ipaddr#119 @reynir, restoring 4.08 compatibility mirage/ocaml-ipaddr#121)
* Provide Ipaddr.Prefix.address (since 5.0.0, Ipaddr.V4.Prefix.address and
  Ipaddr.V6.Prefix.address have been provided, but Ipaddr.Prefix.address was
  forgotten) (mirage/ocaml-ipaddr#122 @hannesm)
* Fix further docstrings (mirage/ocaml-ipaddr#123, fixes mirage/ocaml-ipaddr#81, @hannesm @reynir)
@hannesm hannesm mentioned this issue May 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[new release] ipaddr (6 packages) (5.6.0) hannesm/opam-repository
4 participants
@avsm @hannesm @reynir @cfcs