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
Allow external impls of Bits and BitFlags #348
Comments
As for pub trait Bits:
Clone
+ Copy
+ BitAnd<Output = Self>
+ BitOr<Output = Self>
+ BitXor<Output = Self>
+ Not<Output = Self>
+ PartialEq
+ Sized
+ 'static
{
/// The value of `Self` where no bits are set.
const EMPTY: Self;
/// The value of `Self` where all bits are set.
const ALL: Self;
} The |
The main thing you don't get from just the impl_consts! {
MyFlags: u32 {
const A = 0b0000_0001;
const B = 0b0000_0010;
const C = 0b0000_0100;
}
} would generate: impl MyFlags {
const FLAGS: &'static [(&'static str, Self::Bits)] = &[
("A", 0b0000_0001),
("B", 0b0000_0010),
("C", 0b0000_0100),
];
pub const A: Self = Self::from_bits_retain(0b0000_0001);
pub const B: Self = Self::from_bits_retain(0b0000_0010);
pub const C: Self = Self::from_bits_retain(0b0000_0100);
} you could then use |
Some other open design questions are:
I'm happy with |
Ok, I think I've got a design that should resolve #351, and avoid needing to add direct support for unstable libraries like #325. The idea is you define your flags type manually, with the underlying bits type as a field, and then just use the // First: Define your flags type. It needs to be a newtype over its underlying bits type
pub struct ManualFlags(u32);
// Next: use `impl Flags` instead of `struct Flags`
bitflags! {
impl ManualFlags: u32 {
const A = 0b00000001;
const B = 0b00000010;
const C = 0b00000100;
const ABC = Self::A.bits() | Self::B.bits() | Self::C.bits();
}
} |
Nice! Personally I'm liking the look of that a lot. While admirable that you were willing to do implementations for all those crates, I think leaving it up to the end user will be more flexible and probably cause a lot less version churn in this crate. I could see cases where I would want to just serialize as a u8 for network bandwidth reasons, and other cases where the human readable format makes more sense, so having the choice is great! |
Yeh I think this changes the calculus of what libraries we’ll support directly going forwards to just those that are ubiquitous, and have something to gain from knowing the type is a flags enum. For others, you can now implement them yourself with minimal loss of functionality. It’s better for everyone this way I think. |
@KodrAus Thanks! That will massively reduce the burden on Really curious how you'll expose the flags-aware
@paul-hansen wasn't this already implemented in bitflags 2 where the serializer "detects" whether the target format is binary or intended to be "user readable" with strings? https://github.com/bitflags/bitflags/releases/tag/2.0.0
|
@MarijnS95, the way I've done this in #351 is by exposing functions from /// Serialize a set of flags as a human-readable string or their underlying bits.
pub fn serialize<B: Flags, S: Serializer>(
flags: &B,
serializer: S,
) -> Result<S::Ok, S::Error>
where
B::Bits: LowerHex + Serialize,
{
// Serialize human-readable flags as a string like `"A | B"`
if serializer.is_human_readable() {
serializer.collect_str(&parser::AsDisplay(flags))
}
// Serialize non-human-readable flags directly as the underlying bits
else {
flags.bits().serialize(serializer)
}
} You don't get automatic impl Serialize for MyFlags {
fn serialize<S: Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
bitflags::serde::serialize(self, serializer)
}
} I'll be sure to add an example of these in #351. |
Surely add an example. It's a bit too verbose for my taste, anything we could do to automate this as part of the Does this new/separate system also leave any reason to have the double-newtype indirection from bitflags 2.0.0? |
This was the case that internal field was originally added for; so that you could #[derive(bitflags_serde::Serialize)]
pub struct MyFlags(u32);
bitflags! {
impl MyFlags: u32 { .. }
} It's starting to get a bit complicated there by needing a few different kinds of macros, but could be experimented with. |
I assume this has already been considered, but I couldn't find any discussion of it, so I figured I'd ask just in case. Is there a reason that you couldn't just copy user-supplied attributes to the generated bitflags! {
#[derive(zerocopy::FromBytes)]
struct Flags: u32 {
const A = 0b00000001;
const B = 0b00000010;
const C = 0b00000100;
const ABC = Self::A.bits() | Self::B.bits() | Self::C.bits();
}
} This has the advantage of not requiring Here's the diff. Obviously this is nowhere near complete or correct, and breaks lots of tests and stuff, but I just wanted to confirm that something like this would work in principle.
|
Friendly ping on this 🙂 I'd like to figure out how zerocopy can support bitflags, as a lot of our users use bitflags. |
The
BitFlags
trait is currently sealed, and is only supported through thebitflags!
macro. I think we should make this trait publicly implementable, and default most of its members. I spent some time hacking on this, and came up with this minimal implementation.Given a flags type like:
You can implement it manually with:
I'm proposing we don't do #293, so that the implementation of
BitFlags
doesn't call for a host of supertraits.The
FLAGS
constant there is new, and drives the iteration-based methods likefrom_bits_truncate
,from_name
, and the implementations ofIter
andIterNames
. If you squint, it looks a lot like the body of thebitflags
macro.I think doing this has a few benefits:
$crate::__private::core::option::Option::Some<T>
.InternalBitFlags
fmt
/FromStr
impls? #347 and can't use private derive macros #339, where you might not be able to use thebitflags!
macro, but with a small amount of effort, can cook up your own flags type with whatever shape you want, and still get the benefits of generated code.For trait impls like
serde
andarbitrary
, we can then expose utilities like that proposedbitflags::iter
module that make manual impls of those traits with awareness that the type is a flags enum easy:I'm keen to hear what people think of this.
The text was updated successfully, but these errors were encountered: