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

Fix eio_unix_is_blocking #505

Merged
merged 1 commit into from
May 4, 2023

Conversation

patricoferris
Copy link
Collaborator

@patricoferris patricoferris commented May 3, 2023

I think the stubs were previously incorrect as the == had higher precedence than the & so it was always returning false, I double checked with a small program:

external eio_is_blocking : Unix.file_descr -> bool = "eio_unix_is_blocking"

let () =
  let r, _ = Unix.pipe () in
  print_endline (string_of_bool @@ eio_is_blocking r);
  Unix.set_nonblock r;
  print_endline (string_of_bool @@ eio_is_blocking r)

Which printed false false. You can see the apple C compiler complaining about it here: https://github.com/ocaml-multicore/eio/actions/runs/4870762082/jobs/8686910729.

Afaict old programs using eio_posix shouldn't actually be impacted in terms of correctness as this was more of an optimisation to see if we need to wait for an FD to be readable/writeable.

Copy link
Collaborator

@talex5 talex5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops!

@talex5 talex5 merged commit a92f329 into ocaml-multicore:main May 4, 2023
@talex5
Copy link
Collaborator

talex5 commented May 4, 2023

Note: @haesbaert has some ideas on how to improve warnings from C #448 (comment)

@talex5
Copy link
Collaborator

talex5 commented May 5, 2023

Adding a dune-workspace file with the following seems to do the trick:

(lang dune 3.7)
(env (_ (c_flags :standard -Wall -Werror)))

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.

2 participants