Skip to content
This repository has been archived by the owner on Nov 9, 2019. It is now read-only.

Use cvt from the cvt crate in winx #117

Merged
merged 1 commit into from
Oct 15, 2019
Merged

Conversation

marmistrz
Copy link
Member

cvt was copied over from Rust libstd in #42, but I've created a separate crate for it, so as to be able to reuse it across crates.

@pchickey
Copy link
Contributor

pchickey commented Oct 8, 2019

Hi, I'm pretty ignorant of all things windows programming, and unfortunately I've read this PR and the entirety of the cvt crate and I still don't know what cvt stands for or why it is used. Can you add two sentences of docs to the cvt crate explaining what its about, and why we're using a copy from libstd as a crate?

@programmerjake
Copy link

It would appear as the cvt function handles translating return values from Win32/Unix/VxWorks APIs to std::io::Result.

@programmerjake
Copy link

cvt_r handles retrying the API call when it returns ErrorKind::Interrupted

@marmistrz
Copy link
Member Author

I also have no idea why cvt was named that way :)

What @programmerjake said is exactly what cvt does. Different systems (especially Windows vs Unix) do it in a slightly different way (0 signifies success on Unix but failure on Windows), so cvt is there to reduce the mental bookkeeping and make it easier to handle errors from syscalls.

I've just realized that the crate has no crate-level documentation, but there's some description in the git repo. Is that one okay or should be a little more verbose in the crate description?

We're using a copy from libstd, because libstd::sys is not public. :) Should I also mention this in the docs?

@pchickey
Copy link
Contributor

pchickey commented Oct 9, 2019

That's very helpful. thank you! Those details would be very helpful in the crate docs.

@marmistrz
Copy link
Member Author

I have updated the crate docs and published a new version of the crate.

Copy link
Member

@kubkon kubkon left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@kubkon kubkon merged commit 5dad532 into CraneStation:master Oct 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants