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

Export, document, and use Unix._exit #9914

Merged
merged 3 commits into from
Sep 14, 2020
Merged

Conversation

xavierleroy
Copy link
Contributor

This is a wrapper around the _exit system call. It has been implemented in otherlibs/unix/exit.c for a long time but never exported. This PR exports and documents it as Unix._exit.

The Unix implementation of establish_server is changed to use _exit and to have gender-neutral comments.

A test was added to check that OCaml finalization actions are not performed.

This is a wrapper around the _exit system call.  It has been implemented
in otherlibs/unix/exit.c for a long time but never exported.

This commit exports and documents it as `Unix._exit`.

The Unix implementation of `establish_server` is changed to use `_exit`
and to have gender-neutral comments.

A test was added to check that OCaml finalization actions are not performed.
Copy link
Member

@ivg ivg left a comment

Choose a reason for hiding this comment

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

I only wonder if _exit needs to be added to win32unix.

@shindere
Copy link
Contributor

shindere commented Sep 14, 2020 via email

Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

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

Looks good to me too - the explicit example of its use with fork implicitly covers my previous comment that it's almost certainly not wanted on Windows.

One small question about the test, but it doesn't actually block its merging.


let _ =
at_exit (fun () -> print_string "B\n"; flush stdout);
print_string "A\n"; (* don't flush *)
Copy link
Member

Choose a reason for hiding this comment

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

I thought that the channel I/O doesn't guarantee to flush without an explicit instruction, but isn't it (or a future implementation) at liberty to flush if it wants to? Isn't the test sufficient without this line anyhow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this test relies on a particular implementation of I/O channels w.r.t. flushing, but said behavior hasn't changed since Caml Light, IIRC, so the test should be OK. I'll fix it later if it causes problems.

@dra27
Copy link
Member

dra27 commented Sep 14, 2020

@ivg - exit.c gets copied from unix to win32unix during the build.

@Octachron
Copy link
Member

Beware that UnixLabels.mli has not been updated.

@dra27
Copy link
Member

dra27 commented Sep 14, 2020

Good point - and therefore that #9745 wants to go in first!

@xavierleroy xavierleroy merged commit ba0a9c2 into ocaml:trunk Sep 14, 2020
@xavierleroy xavierleroy deleted the unix__exit branch September 14, 2020 17:03
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

5 participants