Skip to content

Iterator over all the enabled options#278

Merged
KodrAus merged 10 commits into
bitflags:mainfrom
arturoc:iterators
Apr 28, 2022
Merged

Iterator over all the enabled options#278
KodrAus merged 10 commits into
bitflags:mainfrom
arturoc:iterators

Conversation

@arturoc

@arturoc arturoc commented Apr 19, 2022

Copy link
Copy Markdown

Adds a new function that returns an iterator over all the enabled flags.

This implementation takes into account combined flags like the ones described in: #204 (comment)

It uses an impl Iterator as return type because using an specific Iter type implies using const generics which are not supported in earlier versions of rust and adding a const to the trait in order to express the correct dimension of the array as can be seen in the last solution proposed in #204.

Superseeds #204

arturoc added 2 commits April 19, 2022 15:05
This adds a new function that returns an iterator over all the enabled flags.

This implementation takes into account combined flags like the ones described in: bitflags#204 (comment)

It uses an `impl Iterator` as return type because using an specific Iter type implies using const generics which are not supported in earlier versions of rust and adding a const to the trait in order to express the correct dimension of the array as can be seen in the last solution proposed in bitflags#204.

This superseeds bitflags#204
@KodrAus

KodrAus commented Apr 20, 2022

Copy link
Copy Markdown
Member

Ah this is an interesting approach 🤔 Thanks for exploring this @arturoc! I'd love to see some test cases that demonstrate how it works, and ideally we'd be able to replace our debug implementation with it (that might require an implementation that yielded &'static strs instead of flags though, but we could call that iter_names or something.

@arturoc

arturoc commented Apr 20, 2022

Copy link
Copy Markdown
Author

Ah yes I forgot to bring the tests from the previous implementation, just pushed them in a new commit

@arturoc

arturoc commented Apr 20, 2022

Copy link
Copy Markdown
Author

Note how in the first test the flags appear in the wrong order due to the optimization to not go through all the options in every iteration. Removing that would show the flags in the correct order but would be slower for bitflags with a lot of options. I can try to do some benchmarks to understand if it's worth

@arturoc

arturoc commented Apr 20, 2022

Copy link
Copy Markdown
Author

Just figured a better way to optimize the iterator method so it preserves the order of the flags and the options array is a const now.

Comment thread src/lib.rs
struct Flags: u32 {
const ONE = 0b001;
const TWO = 0b010;
const THREE = 0b100;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it be possible to add a cfg'd-out flag to this as well just to make sure we handle those? Something like:

#[cfg(windows)]
const FOUR_WIN = 0b1000;
#[cfg(unix)]
const FOUR_UNIX = 0b10000;

and then use those matching cfgs to check that we iterate over those flags when they're available, but don't when they're not?

@KodrAus

KodrAus commented Apr 23, 2022

Copy link
Copy Markdown
Member

Thanks for working on this @arturoc! It's a nice simple approach you've ended up with. I like it!

@arturoc

arturoc commented Apr 26, 2022

Copy link
Copy Markdown
Author

Thanks! have added a test for cfg

@KodrAus KodrAus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nicely done! This looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants