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 SigFlags into an enum. #460

Merged
merged 4 commits into from
Nov 15, 2016
Merged

Change SigFlags into an enum. #460

merged 4 commits into from
Nov 15, 2016

Conversation

chaosagent
Copy link
Contributor

Addresses #459.

This is a breaking change.

Should SigFlags be renamed to something more sensible?

@fiveop
Copy link
Contributor

fiveop commented Nov 7, 2016

The PR looks good.

I agree that the name should be different. I cannot think of a good naming convention that would work for all enumerations (e.g. SigEnum just does not tell us anything). Therefore, I would propose to go with SigHow.

In addition, could you please add an entry to CHANGELOG.md describing the breaking change.

@chaosagent
Copy link
Contributor Author

I was thinking of something like SigmaskHow since this enum only applies to to the pthread_sigmask and the sigmask (unimplemented in nix) calls but not the rest of the signal functions. In contrast, SigSet applies to both sigaction and sigmask, so it deserves to be labelled generically as Sig. Another possibility is to just use Request as the standard way to specify how-like values in the library like with PtraceRequest, which is actually enumerated in C libc [1], but using How might be advantageous in that it reflects the argument names in libc. Thoughts?

I'm probably making way too much of a fuss about naming though ;)

I'll hold off on the CHANGELOG changes until we figure this out I guess.
[1] http://man7.org/linux/man-pages/man2/ptrace.2.html

@chaosagent
Copy link
Contributor Author

I went ahead and pushed a rename to SigmaskHow and an entry in CHANGELOG.md.

Copy link
Contributor

@fiveop fiveop left a comment

Choose a reason for hiding this comment

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

One last tiny request.

@@ -36,6 +36,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).
accessible with the new method `events()` of `EpollEvent`. Instances of
`EpollEvent` can be constructed using the new method `new()` of EpollEvent.
([#410](https://github.com/nix-rust/nix/pull/410))
- `SigFlags` in `::nix::sys::signal` has be renamed to `SigmaskHow` and its type
has changed from `bitflags` to `enum` in order to conform to our conventions.
([#410](https://github.com/nix-rust/nix/pull/460))
Copy link
Contributor

Choose a reason for hiding this comment

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

Link name is #410 instead of #460.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll just fix it in a separate commit.

@fiveop
Copy link
Contributor

fiveop commented Nov 15, 2016

@homu r+

@homu
Copy link
Contributor

homu commented Nov 15, 2016

📌 Commit e19f142 has been approved by fiveop

@homu
Copy link
Contributor

homu commented Nov 15, 2016

⌛ Testing commit e19f142 with merge 8cda34d...

homu added a commit that referenced this pull request Nov 15, 2016
Change SigFlags into an enum.

Addresses #459.

This is a breaking change.

Should SigFlags be renamed to something more sensible?
@homu
Copy link
Contributor

homu commented Nov 15, 2016

☀️ Test successful - status

1 similar comment
@homu
Copy link
Contributor

homu commented Nov 15, 2016

☀️ Test successful - status

@homu homu merged commit e19f142 into nix-rust:master Nov 15, 2016
This was referenced Nov 15, 2016
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.

3 participants