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

retire ocplib-endian. Since OCaml 4.01 the primitives are part of Bigarray #177

Merged
merged 2 commits into from
Nov 17, 2017

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented Oct 1, 2017

in the same spirit as inhabitedtype/faraday#24 (thanks @seliopou) we can directly use the primitives provided by the OCaml runtime (since 4.01, https://caml.inria.fr/mantis/view.php?id=5771 -- cstruct is only available on >= 4.02.3).

Why? Well, less dependencies are always better. ocplib-endian is additionally LGPL licensed (see https://github.com/OCamlPro/ocplib-endian/blob/master/COPYING.txt) - thus pretty deep down in our dependency chain a not very permissively licensed library.

What do we loose? big endian support (which AFAICT hasn't been used anyways, or has it?) -- it can be re-added by either some preprocessing (cpp0?) or by dynamic checks for Sys.big_endian. I don't know enough about cpp0 do go that way, but I can add the dynamic Sys.big_endian checks if this is desired.

EDIT: I added Sys.big_endian tests, performance could be improved using a preprocessor (but since ocplib as well uses runtime checks, there's no performance regression in here)

@hannesm
Copy link
Member Author

hannesm commented Oct 1, 2017

locally I observed breakage in git, fixed in mirage/ocaml-git#233

@avsm
Copy link
Member

avsm commented Oct 2, 2017

lgtm; i've started the git constraining PR to prepare this for release. I believe that Sys.big_endian can be inlined if it's sufficiently at the toplevel that the inliner picks it up. It might be worth experimenting with the conditional into a toplevel value and looking at the lambda output to see if it's still there after compilation.

@hannesm
Copy link
Member Author

hannesm commented Oct 12, 2017

is there anything I can do to move this PR forward?

@djs55 djs55 merged commit 1f7d12f into mirage:master Nov 17, 2017
@hannesm hannesm deleted the no-ocplib branch November 17, 2017 14:42
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.

None yet

3 participants