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

Remove unused "primitive" #6

Merged
merged 1 commit into from
Jun 13, 2022

Conversation

dra27
Copy link
Contributor

@dra27 dra27 commented Jun 13, 2022

While testing ocaml/ocaml#10926, I discovered this unused "primitive". There's not enough git history to see what was going on, but this looked highly suspect - unix_error_of_code expects a C integer which is tricky to provide from OCaml!

It's unused, so I assume it can be safely deleted.

This is in fact a C function, so if it was ever being used it required
manually converting the OCaml integer being passed to it!

Signed-off-by: David Allsopp <david.allsopp@metastack.com>
@edwintorok
Copy link
Collaborator

edwintorok commented Jun 13, 2022

Thanks, that may have been a leftover from some debugging I've done while writing this. I think some of these functions return an int with a meaning of errno instead of setting errno (so the usual code that raises exceptions when errno is set cannot be used), but I've fixed that in some generic wrapper code here which takes the int returned by the function and converts it and raises properly:

ocaml-dlm/lib/dlm.ml

Lines 20 to 25 in d4d37fa

let fail_errno ?(call="") ?(label="") errno =
let open Errno in
Errno_unix.to_unix_exn (Error {
errno = of_code ~host errno;
call; label;
}) |> Lwt.fail

It is using the Errno_unix module to do that, so this primitive can indeed be dropped.

@edwintorok
Copy link
Collaborator

edwintorok commented Jun 13, 2022

Not sure what is wrong with the opam cache error in the CI (HTTP 503), but this change is simple enough that I'll merge it without waiting on the CI to fix itself.

@edwintorok edwintorok merged commit 3c382b8 into xapi-project:master Jun 13, 2022
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