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

Change gethostname to use a buffer of MaybeUninit values #1745

Merged
merged 2 commits into from
Jul 10, 2022

Conversation

nathaniel-daniel
Copy link
Contributor

Changing gethostname to accept a buffer of MaybeUninit bytes allows the user to avoid needlessly initializing a buffer. This is a breaking API change.

@nathaniel-daniel nathaniel-daniel force-pushed the maybeuninit-gethostname branch 2 times, most recently from 87b7bb3 to b47686e Compare June 15, 2022 02:38
@asomers
Copy link
Member

asomers commented Jun 15, 2022

Certainly an improvement. But wouldn't it be even better if it just returned an owned string, like what ptsname does?

@nathaniel-daniel
Copy link
Contributor Author

I actually am using this for in a wrapper that returns an owned string. I wasn't sure if changing this to only work for owned strings would be an improvement, as its technically possible to take the user-created buffer and unsafely make it owned with the length data from the returned c-string. This method also allows users to use buffers they may already have.

Making this wrapper work only for owned strings would be the most convenient for me, but it would break use cases where the user already has a buffer. I wasn't sure if such a change would be accepted, so I opted to make the simplest, least controversial change. I would be willing to change my PR so that it only works for owned buffers if that is deemed important.

@asomers
Copy link
Member

asomers commented Jun 24, 2022

I think owned strings would be most convenient for everybody. It wouldn't be the most performant option, but how many people rely on maximum performance for gethostname ?

@nathaniel-daniel
Copy link
Contributor Author

Fair enough. I've updated the PR to return an OsString. I'm fairly certain hostnames need to be ascii, but I wasn't able to find a solid source for that. Using an OsString also mirrors sethostname.

@nathaniel-daniel
Copy link
Contributor Author

CI failures seem to be due to cross v0.2.2 now requiring edition 2021.

@rtzoeller
Copy link
Collaborator

@nathaniel-daniel it should be fixed if you rebase!

@nathaniel-daniel
Copy link
Contributor Author

Rebased

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

bors r+

@bors bors bot merged commit c8ffe26 into nix-rust:master Jul 10, 2022
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

3 participants