Implement Iterator for bitflags

[arturoc]

Adds a new iter() method to bitflags that returns an iterator over all the enabled flags.

Uses iter::from_fn to create the iterator to avoid having to add a new struct to the macro which would require a parameter specifying it's name.

[KodrAus]

Thanks for the PR @arturoc! I'm not sure about introducing an impl Trait returning method on the generated flags types themselves here. Perhaps we could add a free-function to the bitflags crate to iterate them?

pub fn iter<B>(flags: B) -> Iter<B> {
    ..
}

pub struct Iter<B> {
    flags: B,
    idx: usize,
}

impl<B> Iterator for Iter<B>
where
    B: BitFlags,
{
    type Item = B;

    ..
}

That would depend on #220, but would be a good usecase for that trait.

[arturoc]

Yes i changed it since then but never sent a PR cause it depends on the BitFlags trait PR. Using that I have something like:

pub struct Iter<B,T> {
    iter_value: B,
    marker: _core::marker::PhantomData<T>,
}

impl<B,T> Iter<B,T> {
    pub fn new(iter_value: B) -> Iter<B,T> {
        Iter {
            iter_value,
            marker: _core::marker::PhantomData,
        }
    }
}

macro_rules! impl_iterator {
    ($t:ty) => {
        impl<B: BitFlags<$t>> Iterator for Iter<B, $t> {
            type Item = B;

            fn next(&mut self) -> Option<B> {
                if self.iter_value.is_empty() {
                    None
                }else{
                    let bits = 1 << self.iter_value.bits().trailing_zeros();
                    let r = unsafe{ B::from_bits_unchecked(bits) };
                    self.iter_value.remove(r);
                    Some(r)
                }
            }
        }
    };
}

impl_iterator!(isize);
impl_iterator!(i8);
impl_iterator!(i16);
impl_iterator!(i32);
impl_iterator!(i64);
impl_iterator!(i128);
impl_iterator!(usize);
impl_iterator!(u8);
impl_iterator!(u16);
impl_iterator!(u32);
impl_iterator!(u64);
impl_iterator!(u128);

the macro and impl_* calls can be removed by using num_traits but i thought that adding more dependencies is probably worse than limiting it to the primitive integer types

[KodrAus]

I’ve been thinking the best approach here might be to add a Iter associated type to the BitFlags trait that assumes the Bits type is a “number” and handles the conversion from the underlying bit type to the flags type. That way we could do it without requiring any unsafe with from_bits_unchecked and still consider #222

[konsumlamm]

What should happen for bitflags like the following?

bitflags! {
    struct Flags: u8 {
        const A = 0b00000001;
        const BC = 0b00000110;
    }
}

In that case, 0b00000010 and 0b00000100 would not be valid flags (but the current implementation would produce them afaict).

Another case to consider is something like the first example in the docs:

bitflags! {
    struct Flags: u32 {
        const A = 0b00000001;
        const B = 0b00000010;
        const C = 0b00000100;
        const ABC = Self::A.bits | Self::B.bits | Self::C.bits;
    }
}

Should an iterator over that yield only A, B and C, or also ABC?

[KodrAus]

@konsumlamm Hmm, those are good questions! I think we'll need to get some input from users who want to iterate flags to get an idea of what they'd expect.

[KodrAus]

The debug implementation uses an approach for determining what flags are set that avoids the issue of considering each bit as an independent flag.

It doesn’t solve the ABC issue though. That’s also brought up in #215. I think we could come up with some “ok” solution to that, but am a little concerned about leaning more heavily on it for anything that’s not purely informational. I wouldn’t be surprised if it was simply infeasible to universally calculate a precise set of flags for a given set of bits.

[KodrAus]

Thinking about this, I think the most appropriate option for an iterator would be to include all flags that are part of the value, whether or not they overlap. This is exactly the debug implementation we have today.

[arturoc]

I've uploaded a new implementation that uses an Iter type to store all the possible combinations and check flag by flag which ones are valid instead of using bit displacement.

It has an optimization to remove flags that have already been checked which makes the order of iteration different than the order in which the flags are declared. The optimization is probably not very useful except for bitflags with many flags so not sure what's preferable.

Also this implementation uses const generics to store the array of possible flags so it would require a higher version of rustc, 1.51 instead of 1.46. I guess this particular functionality could be feature gated and only enabled if the compiler version is higher than that