-
Notifications
You must be signed in to change notification settings - Fork 654
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
conventions: Codify use of newtypes #291
Conversation
```rust | ||
pub struct SigSet(libc::sigset_t); | ||
|
||
impl SigSet { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the right thing to suggest for naming standards is though. Should this be Sigset?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SigSet
feels right to me in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. So, let's add another sentence that mentions that we use the rust convention of camel case names for structs. Then we can merge this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how's the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change looks good to me.
r? @fiveop |
ad38efe
to
dcca871
Compare
dcca871
to
6f0bcde
Compare
One thing I've wondered about is pub struct SigSet {
sigset: libc::sigset_t,
} vs pub struct SigSet(libc::sigset_t); vs pub struct SigSet {
inner: libc::sigset_t,
} There's also a possibility of introducing a something like the Rust project's I brought some of this up in review of #311. |
📌 Commit 6f0bcde has been approved by |
⚡ Test exempted - status |
conventions: Codify use of newtypes
No description provided.