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

Replace ffi module by libc functions in mqueue.rs #392

Merged
merged 3 commits into from
Sep 1, 2016
Merged

Replace ffi module by libc functions in mqueue.rs #392

merged 3 commits into from
Sep 1, 2016

Conversation

fiveop
Copy link
Contributor

@fiveop fiveop commented Aug 3, 2016

This is almost finished. I still need to check if I introduced any breaking changes by changing signatures. I would want to record this in the change log, however, for that we still need to merge #391.

  • update change log
  • run rustfmt on src/mqueue.rs

const O_CREAT = 0o00000100,
const O_EXCL = 0o00000200,
const O_NONBLOCK = 0o00004000,
const O_CLOEXEC = 0o02000000,
Copy link
Member

Choose a reason for hiding this comment

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

Might be better to use the libc constants as the values in the enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it would be better. I will change that.

It would be even better to use OFlag from fcntl.rs. But Oflag has more flags than are allowed for the mqueue functions. I suppose, the other flags would just be ignored if passed into the functions. And just to be safe we could warn about their usage in mqueue functions. I have not made my mind up yet, and this PR is just for getting rid of the ffi module. There is much more that could be done to clean up this module.

@posborne
Copy link
Member

posborne commented Aug 3, 2016

Changes generally look good. Probably run rustfmt on the file while your making changes (a few lines are getting a bit long and its nice to have consistent formatting).

@fiveop
Copy link
Contributor Author

fiveop commented Aug 4, 2016

I'll have a look at rustfmt. I never used it before. (Maybe something for the conventions or contribution guidelines?)

I'll let it run when the rest of this PR is done and add the changes in their own commit.

@fiveop
Copy link
Contributor Author

fiveop commented Aug 6, 2016

I think this can be merged now.

@homu
Copy link
Contributor

homu commented Aug 29, 2016

☔ The latest upstream changes (presumably #402) made this pull request unmergeable. Please resolve the merge conflicts.

@fiveop
Copy link
Contributor Author

fiveop commented Aug 31, 2016

Given that no further comments were made, I'll merge this tomorrow.

@fiveop
Copy link
Contributor Author

fiveop commented Sep 1, 2016

@homu r=@fiveop

@homu
Copy link
Contributor

homu commented Sep 1, 2016

📌 Commit 8ed5521 has been approved by @fiveop

homu added a commit that referenced this pull request Sep 1, 2016
Replace ffi module by libc functions in mqueue.rs

This is almost finished. I still need to check if I introduced any breaking changes by changing signatures. I would want to record this in the change log, however, for that we still need to merge #391.

- [x] update change log
- [x] run rustfmt on `src/mqueue.rs`
@homu
Copy link
Contributor

homu commented Sep 1, 2016

⌛ Testing commit 8ed5521 with merge 45e3170...

@homu
Copy link
Contributor

homu commented Sep 1, 2016

☀️ Test successful - status

@homu homu merged commit 8ed5521 into nix-rust:master Sep 1, 2016
@fiveop fiveop deleted the less_ffi branch October 15, 2016 17:48
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