More cleanups and enable some more compiler lints #837
Conversation
| @@ -205,6 +205,7 @@ libc_bitflags!( | |||
| } | |||
| ); | |||
|
|
|||
| #[allow(missing_debug_implementations)] | |||
There was a problem hiding this comment.
Why do you allow missing debug impls for certain types?
There was a problem hiding this comment.
No libc types implement Debug, so derive(Debug) doesn't work for them. I've decided to defer these implementations for now in order to get these checks working for PRs in general. I'd like to come back and add impl Debug for all types as a future PR.
Susurrus
force-pushed
the
lints
branch
5 times, most recently
from
9b99ebc to
27d986b
Compare
|
The latest version relies on rust-lang/libc#898 being merged, but that's passing test so should happen soon. |
Susurrus
changed the title
|
@asomers This is now passing on all platforms and should be good to merge once the OS X backlog dies down a bit on Travis. Can you take a look-see in the meantime? I only expect 429af5e to possibly be controversial since it adds |
There was a problem hiding this comment.
I don't see a problem with Clone/Copy on any of the types where you added it. They're all plain-old-data types. The only one that could potentially be a problem is OpenptyResult, but given that the public API already promises nothing special on Drop, it's not a problem. It would only be a problem if we significantly reworked the public interface for that type, such as by having it return a pair of File objects instead of RawFds. I found just a few other nits in other changes. Otherwise, the PR looks good.
CHANGELOG.md
Outdated
| @@ -96,6 +96,8 @@ This project adheres to [Semantic Versioning](http://semver.org/). | |||
| ([#814](https://github.com/nix-rust/nix/pull/814)) | |||
| - Removed return type from `pause`. | |||
| ([#829](https://github.com/nix-rust/nix/pull/829)) | |||
| - Display and Debug for SysControlAddr now includes all fields. | |||
| ([#829](https://github.com/nix-rust/nix/pull/829)) | |||
src/features.rs
Outdated
| @@ -82,6 +83,7 @@ mod os { | |||
| } | |||
| } | |||
|
|
|||
| /// Check if the OS supports atmomic close-on-exec for sockets | |||
There was a problem hiding this comment.
s/atmomic/atomic here and on line 99
src/sys/aio.rs
Outdated
| @@ -86,7 +86,7 @@ pub struct AioCb<'a> { | |||
| /// Could this `AioCb` potentially have any in-kernel state? | |||
| in_progress: bool, | |||
| /// Used to keep buffers from Drop'ing | |||
| keeper: Keeper<'a> | |||
| _keeper: Keeper<'a> | |||
There was a problem hiding this comment.
These changes will be unnecessary after and will conflict with #820 . You should revert the changes to this file.
|
I've updated based on your first 2 comments. I'll block this one on the merging of #820 and update this once that's merged. |
Also alter Debug to output all fields.
|
@asomers All good? |
|
Yep. bors r+ |
Gonna run this through CI and see what breaks on non-Linux-x64 platforms.
These changes are minor cleanups, but improve our doc story and should help with future submitted PRs.