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

BitFlags trait #220

Merged
merged 8 commits into from Oct 23, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
68 changes: 68 additions & 0 deletions src/bitflags_trait.rs
@@ -0,0 +1,68 @@
use core as _core;
Copy link
Contributor

@konsumlamm konsumlamm Aug 4, 2021

Choose a reason for hiding this comment

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

Suggested change
use core as _core;

This is only necessary for use in macros, so that types get resolved correctly. For the definition of the trait, you can just write Option etc.


pub trait BitFlags<T>:
Copy link
Member

Choose a reason for hiding this comment

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

We should probably make this an associated type, since an implementation of BitFlags only corresponds to a single underlying integer type.

Copy link
Author

Choose a reason for hiding this comment

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

I think the lines in this comment are wrong? do you mean perhaps empty and all?

Copy link
Member

Choose a reason for hiding this comment

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

Ah sorry, I mean making the trait look like:

pub trait BitFlags {
    type Bits;

    ..
}

instead of:

pub trait BitFlags<T> {
    ..
}

_core::ops::BitOr<Output = Self>
Copy link
Member

Choose a reason for hiding this comment

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

I think it might be better for the BitFlags trait not to include these additional supertraits and let consumers add them as additional bounds if they need them.

On the one hand it seems reasonable that if you've got flags you'll want to perform bitwise operations on them, but on the other it's a bit of a grab-bag snapshot of our current functionality that we won't be able to extend (since we can't add new supertraits without breakage).

It might be best to stick with just the current set of methods on the trait which cover:

  • Construction from bits
  • Conversion back into bits
  • Set operations on pairs of flags

That should be enough to support a wide range of fundamental use-cases.

Copy link
Author

Choose a reason for hiding this comment

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

What about Clone + Copy? I need those to implement the iterator which is based on a value and removes the flags as it advances through it.

Copy link
Member

Choose a reason for hiding this comment

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

Do we need the BitFlags type itself to be Copy? Or just the underlying bits? If I understand right, we could just perform shifts and masks on the bits themselves and use BitFlags::from_bits_unchecked to return just that flag.

+ _core::ops::BitOrAssign
+ _core::ops::BitXor<Output = Self>
+ _core::ops::BitXorAssign
+ _core::ops::BitAnd<Output = Self>
+ _core::ops::BitAndAssign
+ _core::ops::Sub<Output = Self>
+ _core::ops::SubAssign
+ _core::ops::Not<Output = Self>
+ _core::iter::Extend<Self>
+ _core::iter::FromIterator<Self>
+ _core::fmt::Debug
+ _core::cmp::PartialEq<Self>
+ _core::cmp::Eq
+ _core::cmp::PartialOrd<Self>
+ _core::cmp::Ord
+ _core::hash::Hash
+ _core::fmt::Octal
+ _core::fmt::Binary
+ _core::fmt::LowerHex
+ _core::fmt::UpperHex
+ Clone
+ Copy
{
/// Returns an empty set of flags.
fn empty() -> Self;
/// Returns the set containing all flags.
fn all() -> Self;
/// Returns the raw value of the flags currently stored.
fn bits(&self) -> T;
/// Convert from underlying bit representation, unless that
/// representation contains bits that do not correspond to a flag.
fn from_bits(bits: T) -> _core::option::Option<Self>;
/// Convert from underlying bit representation, dropping any bits
/// that do not correspond to flags.
unsafe fn from_bits_truncate(bits: T) -> Self;
Copy link
Member

Choose a reason for hiding this comment

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

This method doesn't need to be unsafe.

/// Convert from underlying bit representation, preserving all
/// bits (even those not corresponding to a defined flag).
///
/// # Safety
///
/// The caller of the `bitflags!` macro can chose to allow or
/// disallow extra bits for their bitflags type.
///
/// The caller of `from_bits_unchecked()` has to ensure that
/// all bits correspond to a defined flag or that extra bits
/// are valid for this bitflags type.
unsafe fn from_bits_unchecked(bits: T) -> Self;
/// Returns `true` if no flags are currently stored.
fn is_empty(&self) -> bool;
/// Returns `true` if all flags are currently set.
fn is_all(&self) -> bool;
/// Returns `true` if there are flags common to both `self` and `other`.
fn intersects(&self, other: Self) -> bool;
/// Returns `true` all of the flags in `other` are contained within `self`.
fn contains(&self, other: Self) -> bool;
/// Inserts the specified flags in-place.
fn insert(&mut self, other: Self);
/// Removes the specified flags in-place.
fn remove(&mut self, other: Self);
/// Toggles the specified flags in-place.
fn toggle(&mut self, other: Self);
/// Inserts or removes the specified flags depending on the passed value.
fn set(&mut self, other: Self, value: bool);
}
63 changes: 63 additions & 0 deletions src/lib.rs
Expand Up @@ -262,6 +262,11 @@ extern crate std;
#[doc(hidden)]
pub extern crate core as _core;

#[doc(inlined)]
pub use bitflags_trait::BitFlags;

mod bitflags_trait;

/// The macro used to generate the flag structure.
///
/// See the [crate level docs](../bitflags/index.html) for complete documentation.
Expand Down Expand Up @@ -814,6 +819,64 @@ macro_rules! __impl_bitflags {
result
}
}

impl $crate::BitFlags<$T> for $BitFlags {
fn empty() -> Self {
Copy link

Choose a reason for hiding this comment

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

You probably want to put inline on most methods here, to enable cross-crate inlining if called via a trait.

$BitFlags::empty()
}

fn all() -> Self {
$BitFlags::all()
}

fn bits(&self) -> $T {
$BitFlags::bits(self)
}

fn from_bits(bits: $T) -> $crate::_core::option::Option<$BitFlags> {
$BitFlags::from_bits(bits)
}

unsafe fn from_bits_truncate(bits: $T) -> $BitFlags {
$BitFlags::from_bits_truncate(bits)
}

unsafe fn from_bits_unchecked(bits: $T) -> $BitFlags {
$BitFlags::from_bits_unchecked(bits)
}

fn is_empty(&self) -> bool {
$BitFlags::is_empty(self)
}

fn is_all(&self) -> bool {
$BitFlags::is_all(self)
}

fn intersects(&self, other: $BitFlags) -> bool {
$BitFlags::intersects(self, other)
}

fn contains(&self, other: $BitFlags) -> bool {
$BitFlags::contains(self, other)
}

fn insert(&mut self, other: $BitFlags) {
$BitFlags::insert(self, other)
}

fn remove(&mut self, other: $BitFlags) {
$BitFlags::remove(self, other)
}

fn toggle(&mut self, other: $BitFlags) {
$BitFlags::toggle(self, other)
}

fn set(&mut self, other: $BitFlags, value: bool) {
$BitFlags::set(self, other, value)
}
}
};

// Every attribute that the user writes on a const is applied to the
Expand Down
13 changes: 12 additions & 1 deletion test_suite/tests/compile-fail/private_flags.stderr
Expand Up @@ -2,4 +2,15 @@ error[E0603]: struct `Flags2` is private
--> $DIR/private_flags.rs:19:26
|
19 | let flag2 = example::Flags2::FLAG_B;
| ^^^^^^
| ^^^^^^ private struct
|
note: the struct `Flags2` is defined here
--> $DIR/private_flags.rs:10:5
|
10 | / bitflags! {
11 | | struct Flags2: u32 {
12 | | const FLAG_B = 0b00000010;
13 | | }
14 | | }
| |_____^
= note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)