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

Add {In,Out}_channel.is_binary_mode #12845

Merged
merged 1 commit into from
Dec 19, 2023
Merged

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Dec 19, 2023

This is a further follow-on to the story of #12792. There are (good) ways of not needing to know the binary mode of a channel, but they mostly involve using the Unix library. In these guilt-free post-#10545 times, it seems we could risk having is_binary_mode as the dual of set_binary_mode in In_channel and Out_channel in the same way as we have Out_channel.is_buffered as the dual of Out_channel.set_buffered. Further, the function to get that mode was already exposed in the runtime's C API.

@dra27 dra27 added the stdlib label Dec 19, 2023
Copy link
Contributor

@nojb nojb left a comment

Choose a reason for hiding this comment

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

LGTM

A second approval is needed, though.

Copy link
Contributor

@xavierleroy xavierleroy 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. I'm still not sure it's useful to be able to set the binary mode after a channel was opened, but it's more useful to be able to check that a channel is in binary mode, in my opinion, so let's make both operations available!

Comment on lines 215 to 216
@since 5.3 *)

Copy link
Contributor

Choose a reason for hiding this comment

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

It might still be time to sneak this into 5.2, with @Octachron 's approval.

Copy link
Member

Choose a reason for hiding this comment

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

That's fine with me.

@dra27
Copy link
Member Author

dra27 commented Dec 19, 2023

Thank you both!

@dra27 dra27 added the merge-me label Dec 19, 2023
@dra27 dra27 added this to the 5.2 milestone Dec 19, 2023
@dra27 dra27 merged commit 30971bf into ocaml:trunk Dec 19, 2023
10 checks passed
@dra27 dra27 deleted the get_binary_mode branch December 19, 2023 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants