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

Remove revents from PollFd::new #542

Merged
merged 1 commit into from
Mar 3, 2017
Merged

Remove revents from PollFd::new #542

merged 1 commit into from
Mar 3, 2017

Conversation

Susurrus
Copy link
Contributor

I could've used a 0i16here as well but I liked the better semantics of empty().

@kamalmarhubi
Copy link
Member

@Susurrus can you explain this a tiny bit more? I am guessing that revents is populated by poll et al and so it doesn't make sense for us to set it?

@kamalmarhubi
Copy link
Member

Ah yeah, that looks to be the case:

struct pollfd {
    int   fd;         /* file descriptor */
    short events;     /* requested events */
    short revents;    /* returned events */
};

@Susurrus could you add a tiny bit of context to the commit message, and add this to the Changed section in the changelog?

@Susurrus
Copy link
Contributor Author

Susurrus commented Mar 1, 2017

@kamalmarhubi Exactly. I've updated the changelog and added context to the commit message. Is there any way I should denote this being a breaking change, either in the commit, changelog, or somewhere else? Since nix is pre-1.0, I don't think it matters much, but eventually breaking changes will need to be tracked for proper semver.

@Susurrus
Copy link
Contributor Author

Susurrus commented Mar 1, 2017

Re breaking changes that's already got an issue #458, which doesn't seem to ascribe any specific action for breaking changes, so I'll assume there's nothing else I need to do and defer higher-level discussion to that issue.

revents is an output field so regardless of what value it is set to it
will be overwritten by many of the function calls that take a PollFd.
The only value that makes sense for the caller to pass in in
`EventFlags::empty()` so we just hardcode that instead of making the
caller do it.
@Susurrus
Copy link
Contributor Author

Susurrus commented Mar 3, 2017

@kamalmarhubi Should be good to go now!

@fiveop
Copy link
Contributor

fiveop commented Mar 3, 2017

@homu r+

@homu
Copy link
Contributor

homu commented Mar 3, 2017

📌 Commit 80453b9 has been approved by fiveop

@homu
Copy link
Contributor

homu commented Mar 3, 2017

⌛ Testing commit 80453b9 with merge 5c90289...

homu added a commit that referenced this pull request Mar 3, 2017
Remove revents from PollFd::new

I could've used a `0i16`here as well but I liked the better semantics of `empty()`.
@homu
Copy link
Contributor

homu commented Mar 3, 2017

☀️ Test successful - status

@homu homu merged commit 80453b9 into nix-rust:master Mar 3, 2017
@Susurrus Susurrus deleted the revents branch March 3, 2017 20:58
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.

4 participants