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
Minor clean-ups in runtime/io.c and runtime/caml/io.h #10136
Conversation
Cash (http://pauillac.inria.fr/cash/) was an experiment in using OCaml for shell scripting. The project is no longer active and the old code does not compile with OCaml 4.
Commit-by-commit: the The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wondered about the possible impact of the change to caml_all_opened_channels
, and concluded that it is fine because opening and closing channels from C is not exposed in the public API. It all looks good to me (apart from @dra27's pending caml_thread_reinitialise
question above). A minor comment below.
runtime/caml/io.h
Outdated
int revealed; /* For Cash only */ | ||
int old_revealed; /* For Cash only */ | ||
int refcount; /* For flush_all and for Cash */ | ||
int refcount; /* For flush_all */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refcount
can go too. It is only increased inside caml_alloc_channel
to prevent a race with finalisation (#8191), but finalisation no longer happens during allocation from C.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upon closer inspection refcount
needs to stay, but the comment /* prevent finalization during next alloc */
should be fixed to better match the purpose of the reference count :)
The test for refcount > 0
inside caml_close_channel
is now pointless and can be changed into an assertion refcount == 0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would document
int refcount; /* Number of custom blocks owning the struct */
and remove CHANNEL_FLAG_MANAGED_BY_GC
.
Here it looks like it is? |
Oops, I either didn’t scroll up far enough or was being ditsy - the struct is indeed internal! |
Control flow can be simplified, removing the `do_syscall` variable.
To match `Val_emptylist`.
These I/O operations used to be defined as macros in <caml/io.h>, but they are not speed-critical, so it's cleaner to have them as functions, like all other I/O operations in this file.
5267953
to
20e597d
Compare
Thank you @dra27 and @gadmm for the helpful comments.
Well spotted. Maybe one day we'll use such channels in a multithreaded context. I changed the implementation to keep all channels in the C list I updated the comments following @gadmm's suggestions. One last cleanup: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A whitespace nit (I assume?) - I guess you were going to push Changes just prior to merging (the changes are internal, but probably worth having one for naughty people who define CAML_INTERNALS
and get singed...)
runtime/io.c
Outdated
caml_ml_close_channel changes max when setting fd to -1. */ | ||
if (channel->max == NULL) { | ||
if (channel -> max == NULL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (channel -> max == NULL | |
if (channel->max == NULL |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oups! Apologies for this strange typo. I guess those spaces were destined for another window but windows focus wasn't where I expected it... I squashed the fix with the original commit, so that it doesn't show up.
Looks good to me too, but I have two comments:
|
Previously, output channels opened from C were also reported in this list, causing `Stdlib.flush_all` to flush them. This feels wrong, as channels opened from C are entirely managed from C code. With this commit, output channels opened from C always have a reference count of 0: `caml_alloc_channel` is no longer called with one of these channels, and it's the only function that increases refcount. Hence the refcount check in `caml_close_channel` can be removed.
`caml_allc_custom_mem` cannot trigger finalization any longer.
85d1fde
to
691655c
Compare
I think you're right, but to me this wouldn't make the code clearer. When there is no clear advantage to a code change, I tend to keep the original code.
The main uses of these functions outside io.c is in the debugger interface, so that's not critical. On the other hand there are some uses in io.c ( This would be a fine use of "extern inline" C99 functions, but I don't know how to achieve the same effect with MSVC. Plus, keeping the macros (just moving them from io.h to io.c) makes the change smaller. |
Changes entry added. CI is good. Merging! |
Thanks for the replies, it would be nice to know for extern+inline functions. I was already wondering the portability of this for something unrelated and I was puzzled that I could not already find instances of it in the codebase. |
… rebase Mentioned in ocaml#10831 review comment on runtime/io.c:552.
While investigating #9786, I came across some minor clean-ups in the buffered I/O code:
Stdlib.flush_all
caml_ml_close_channel
andcaml_ml_out_channels_list
can be written more clearly.