From c5a82cf6757ed4256a02218649ad1c213c9da240 Mon Sep 17 00:00:00 2001 From: Bryant Mairs Date: Tue, 5 Dec 2017 20:50:51 -0800 Subject: [PATCH 1/3] Expand traits implemented by structs within libc --- text/0000-libc-struct-traits.md | 85 +++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100644 text/0000-libc-struct-traits.md diff --git a/text/0000-libc-struct-traits.md b/text/0000-libc-struct-traits.md new file mode 100644 index 00000000000..14b2475b5da --- /dev/null +++ b/text/0000-libc-struct-traits.md @@ -0,0 +1,85 @@ +- Feature Name: (libc_struct_traits) +- Start Date: (2017-12-05) +- RFC PR: (leave this empty) +- Rust Issue: (leave this empty) + +# Summary +[summary]: #summary + +Expand the traits implemented by structs `libc` crate to include `Debug`, `Eq`, `Hash`, and `PartialEq`. + +# Motivation +[motivation]: #motivation + +This will allow downstream crates to easily support similar operations with any types they +provide that contain `libc` structs. Additionally [The Rust API Guidelines](https://rust-lang-nursery.github.io/api-guidelines/checklist.html) specify that it is +considered useful to expose as many traits as possible from the standard library. In order to facilitate the +following of these guidelines, official Rust libraries should lead by example. + +For many of these traits, it is trivial for downstream crates to implement them for these types by using +newtype wrappers. As a specific example, the `nix` crate offers the `TimeSpec` wrapper type around the `timespec` struct. This +wrapper could easily implement `Eq` through comparing both fields in the struct. + +Unfortunately there are a great many structs that are large and vary widely between platforms. Some of these in use by `nix` +are `dqblk`, `utsname`, and `statvfs`. These structs have fields and field types that vary across platforms. As `nix` aims to +support as many platforms as `libc` does, this variation makes implementing these traits manually on wrapper types time consuming and +error prone. + +# Guide-level explanation +[guide-level-explanation]: #guide-level-explanation + +The feature flag `derive_all` is added to the `libc` library that when enabled adds `Debug`, `Eq`, `Hash`, and `PartialEq` traits for all structs. It will default to off. + +# Reference-level explanation +[reference-level-explanation]: #reference-level-explanation + +The `Debug`, `Eq`, `Hash`, and `PartialEq` traits will be added as automatic derives within the `s!` macro in `src/macros.rs` if the `derive_all` feature +flag is enabled. This won't work for some types because auto-derive doesn't work for arrays larger than 32 elements, so for these they'll be implemented manually. For `libc` +as of `bbda50d20937e570df5ec857eea0e2a098e76b2d` on `x86_64-unknown-linux-gnu` these many structs will need manual implementations: + + * `Debug` - 17 + * `Eq` and `PartialEq` - 46 + * `Hash` - 17 + +# Drawbacks +[drawbacks]: #drawbacks + +The addition of this behind a feature flag does not have a significant effect on build times, but the burden of adding these implementations for new types that +require manual implementations will be high, possibly hindering new contributors. + +# Rationale and alternatives +[alternatives]: #alternatives + +Adding these trait implementations behind a singular feature flag has the best compination of utility and ergonomics out of the possible alternatives listed below: + +## Always enabled + +This was regarded as unsuitable because it doubles to triples compilation time. Compilation times of `libc` was tested at commit `bbda50d20937e570df5ec857eea0e2a098e76b2d` +with modifications to add derives for the traits discussed here. Some types failed to have these traits derived because of specific fields, so these were removed from the +struct declaration. The table below shows the results: + +| Build arguments | Time | +|--------------------------------------------------------------------------------------------|-------| +| `cargo clean && cargo build --no-default-features` | 0.84s | +| `cargo clean && cargo build --no-default-features --features derive_all` | 2.17s | +| `cargo clean && cargo build --no-default-features --release` | 0.64s | +| `cargo clean && cargo build --no-default-features --release --features derive_all` | 1.80s | +| `cargo clean && cargo build --no-default-features --features use_std` | 1.14s | +| `cargo clean && cargo build --no-default-features --features use_std,derive_all` | 2.34s | +| `cargo clean && cargo build --no-default-features --release --features use_std` | 0.66s | +| `cargo clean && cargo build --no-default-features --release --features use_std,derive_all` | 1.94s | + +## Default-on feature flag + +For crates that are more than one level above `libc` in the dependency chain it will be impossible for them to opt out. This could also happen with a default-off +feature flag, but it's more likely the library authors will expose it as a flag as well. + +## Independent feature flags + +It wasn't tested how much compilation times increased per-trait, but further mitigation of slow compilation times could done by exposing all traits mentioned here +behind individual feature flags. By doing this it becomes harder for downstream crates to pass-through these feature flags, so it's likely not a worthwhile tradeoff. + +# Unresolved questions +[unresolved]: #unresolved-questions + +Is `derive_all` a suitable name for this feature flag? From 63eada3e51bab2c5a5addad7792cb102e08a28d1 Mon Sep 17 00:00:00 2001 From: Bryant Mairs Date: Wed, 2 Jan 2019 21:03:02 -0800 Subject: [PATCH 2/3] Revise libc_struct_traits --- text/0000-libc-struct-traits.md | 57 ++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 25 deletions(-) diff --git a/text/0000-libc-struct-traits.md b/text/0000-libc-struct-traits.md index 14b2475b5da..4a78b840f8e 100644 --- a/text/0000-libc-struct-traits.md +++ b/text/0000-libc-struct-traits.md @@ -28,58 +28,65 @@ error prone. # Guide-level explanation [guide-level-explanation]: #guide-level-explanation -The feature flag `derive_all` is added to the `libc` library that when enabled adds `Debug`, `Eq`, `Hash`, and `PartialEq` traits for all structs. It will default to off. +Add an `extra_traits` feature to the `libc` library that enables `Debug`, `Eq`, `Hash`, and `PartialEq` implementations for all structs. # Reference-level explanation [reference-level-explanation]: #reference-level-explanation -The `Debug`, `Eq`, `Hash`, and `PartialEq` traits will be added as automatic derives within the `s!` macro in `src/macros.rs` if the `derive_all` feature +The `Debug`, `Eq`/`PartialEq`, and `Hash` traits will be added as automatic derives within the `s!` macro in `src/macros.rs` if the corresponding feature flag is enabled. This won't work for some types because auto-derive doesn't work for arrays larger than 32 elements, so for these they'll be implemented manually. For `libc` as of `bbda50d20937e570df5ec857eea0e2a098e76b2d` on `x86_64-unknown-linux-gnu` these many structs will need manual implementations: * `Debug` - 17 - * `Eq` and `PartialEq` - 46 + * `Eq`/`PartialEq` - 46 * `Hash` - 17 # Drawbacks [drawbacks]: #drawbacks -The addition of this behind a feature flag does not have a significant effect on build times, but the burden of adding these implementations for new types that -require manual implementations will be high, possibly hindering new contributors. +While most structs will be able to derive these implementations automatically, some will not (for example arrays larger than 32 elements). This will make it harder to add +some structs to `libc`. + +This extra trait will increase the testing requirements for `libc`. # Rationale and alternatives [alternatives]: #alternatives Adding these trait implementations behind a singular feature flag has the best compination of utility and ergonomics out of the possible alternatives listed below: -## Always enabled +## Always enabled with no feature flags -This was regarded as unsuitable because it doubles to triples compilation time. Compilation times of `libc` was tested at commit `bbda50d20937e570df5ec857eea0e2a098e76b2d` -with modifications to add derives for the traits discussed here. Some types failed to have these traits derived because of specific fields, so these were removed from the -struct declaration. The table below shows the results: +This was regarded as unsuitable because it increases compilation times by 100-200%. Compilation times of `libc` was tested at commit `bbda50d20937e570df5ec857eea0e2a098e76b2d` +with modifications to add derives for the traits discussed here under the `extra_traits` feature (with no other features). Some types failed to have these traits +derived because of specific fields, so these were removed from the struct declaration. The table below shows the compilation times: -| Build arguments | Time | -|--------------------------------------------------------------------------------------------|-------| -| `cargo clean && cargo build --no-default-features` | 0.84s | -| `cargo clean && cargo build --no-default-features --features derive_all` | 2.17s | -| `cargo clean && cargo build --no-default-features --release` | 0.64s | -| `cargo clean && cargo build --no-default-features --release --features derive_all` | 1.80s | -| `cargo clean && cargo build --no-default-features --features use_std` | 1.14s | -| `cargo clean && cargo build --no-default-features --features use_std,derive_all` | 2.34s | -| `cargo clean && cargo build --no-default-features --release --features use_std` | 0.66s | -| `cargo clean && cargo build --no-default-features --release --features use_std,derive_all` | 1.94s | +| Build arguments | Time | +|----------------------------------------------------------------------------------------------|-------| +| `cargo clean && cargo build --no-default-features` | 0.84s | +| `cargo clean && cargo build --no-default-features --features extra_traits` | 2.17s | +| `cargo clean && cargo build --no-default-features --release` | 0.64s | +| `cargo clean && cargo build --no-default-features --release --features extra_traits` | 1.80s | +| `cargo clean && cargo build --no-default-features --features use_std` | 1.14s | +| `cargo clean && cargo build --no-default-features --features use_std,extra_traits` | 2.34s | +| `cargo clean && cargo build --no-default-features --release --features use_std` | 0.66s | +| `cargo clean && cargo build --no-default-features --release --features use_std,extra_traits` | 1.94s | -## Default-on feature flag +## Default-on feature For crates that are more than one level above `libc` in the dependency chain it will be impossible for them to opt out. This could also happen with a default-off feature flag, but it's more likely the library authors will expose it as a flag as well. -## Independent feature flags +## Multiple feature flags + +Instead of having a single `extra_traits` feature, have it and feature flags for each trait individually like: -It wasn't tested how much compilation times increased per-trait, but further mitigation of slow compilation times could done by exposing all traits mentioned here -behind individual feature flags. By doing this it becomes harder for downstream crates to pass-through these feature flags, so it's likely not a worthwhile tradeoff. + * `trait_debug` - Enables `Debug` for all structs + * `trait_eg` - Enables `Eq` and `PartialEq` for all structs + * `trait_hash` - Enables `Hash` for all structs + * `extra_traits` - Enables all of the above through dependent features + +This change should reduce compilation times when not all traits are desired. The downsides are that it complicates CI. It can be added in a backwards-compatible +manner later should compilation times or consumer demand changes. # Unresolved questions [unresolved]: #unresolved-questions - -Is `derive_all` a suitable name for this feature flag? From 511630e0250e0d1e085f44bc6b5914b373737040 Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Thu, 17 Jan 2019 23:13:48 +0100 Subject: [PATCH 3/3] RFC 2235 --- ...0-libc-struct-traits.md => 2235-libc-struct-traits.md} | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) rename text/{0000-libc-struct-traits.md => 2235-libc-struct-traits.md} (95%) diff --git a/text/0000-libc-struct-traits.md b/text/2235-libc-struct-traits.md similarity index 95% rename from text/0000-libc-struct-traits.md rename to text/2235-libc-struct-traits.md index 4a78b840f8e..2cf2fbb90d6 100644 --- a/text/0000-libc-struct-traits.md +++ b/text/2235-libc-struct-traits.md @@ -1,7 +1,7 @@ -- Feature Name: (libc_struct_traits) -- Start Date: (2017-12-05) -- RFC PR: (leave this empty) -- Rust Issue: (leave this empty) +- Feature Name: `libc_struct_traits` +- Start Date: 2017-12-05 +- RFC PR: [rust-lang/rfcs#2235](https://github.com/rust-lang/rfcs/pull/2235) +- Rust Issue: [rust-lang/rust#57715](https://github.com/rust-lang/rust/issues/57715) # Summary [summary]: #summary