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

Match syntax of libc_bitflags! with bitflags! #725

Merged
merged 8 commits into from
Aug 18, 2017

Conversation

Susurrus
Copy link
Contributor

@Susurrus Susurrus commented Aug 9, 2017

Also update a couple of constant declarations while we're at it.

@Susurrus Susurrus force-pushed the bitflags branch 9 times, most recently from 8845dc2 to cd21efc Compare August 10, 2017 17:05
@Susurrus
Copy link
Contributor Author

@asomers Want to give this a quick glance-over for me? It should be pretty straight-forward.

@asomers asomers self-requested a review August 11, 2017 04:36
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.

Other than the excessive unsafes, it all looks good to me.

bits: [0; FD_SETSIZE as usize / BITS]
}
let mut fdset = unsafe { mem::uninitialized() };
unsafe { libc::FD_ZERO(&mut fdset) };
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't require unsafe. FD_ZERO is implemented in pure Rust. Same for FD_CLR, FD_SET, and FD_ISSET

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same thing as WIFCONTINUED, this is how they're defined in libc:

pub unsafe extern "C" fn FD_SET(fd: c_int, set: *mut fd_set);

@Susurrus Susurrus force-pushed the bitflags branch 3 times, most recently from 0276742 to 4588d4c Compare August 17, 2017 00:41
@Susurrus
Copy link
Contributor Author

@asomers Want to give this another look over now? Should be RFM.

@asomers
Copy link
Member

asomers commented Aug 17, 2017

LGTM. But can you squash the last commit?

@Susurrus
Copy link
Contributor Author

@asomers Squash that last commit into which other commit? I left it separate because it didn't seem appropriate to merge it with any other changes in this patchset.

@asomers
Copy link
Member

asomers commented Aug 17, 2017

Sorry, I thought that the extra AtFlags definition was also introduced by this PR. But now I see that it wasn't.

bors r+

bors bot added a commit that referenced this pull request Aug 17, 2017
700: Get rid of a lot of transmutes r=asomers

Most could be replaced by simple raw pointer casts (or even perfectly
safe coercions!).

cc #373

725: Match syntax of libc_bitflags! with bitflags! r=asomers

Also update a couple of constant declarations while we're at it.
@bors
Copy link
Contributor

bors bot commented Aug 17, 2017

Timed out (retrying...)

bors bot added a commit that referenced this pull request Aug 17, 2017
725: Match syntax of libc_bitflags! with bitflags! r=asomers

Also update a couple of constant declarations while we're at it.
@bors
Copy link
Contributor

bors bot commented Aug 18, 2017

@bors bors bot merged commit 4e2bf09 into nix-rust:master Aug 18, 2017
@Susurrus Susurrus deleted the bitflags branch August 18, 2017 00:57
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.

2 participants