Bitflags values of zero are considered always present

Alex-Addy commented

2018-03-27 10:37:57 -05:00

(Migrated from github.com)

When a bitflag is built with a zero valued item then it is treated as always present by bitflags functions.

This may cause bugs but it definitely causes confusion when printing such a value. See the following for an example:

#[macro_use]
extern crate bitflags;

bitflags! {
    struct Flags: u32 {
        const NONE = 0b0;
        const SOME = 0b1;
    }
}

fn main() {
    let none = Flags::NONE;
    let some = Flags::SOME;
    
    println!("NONE = {:?}", none);
    println!("SOME = {:?}", some);
    assert!(some.contains(Flags::NONE));
}

Results in an output of:

NONE = NONE
SOME = NONE | SOME

I noticed this when printing some flags from git2, specifically MergeAnalysis. The output of a format("{:?}", flag) ended up being "(ANALYSIS_NONE | ANALYSIS_NORMAL | ANALYSIS_FASTFORWARD)" which confused me greatly. ANALYSIS_NONE is zero as that is how it is defined in git2

It seems to me that way that flag is intended to be used is that NONE only shows up when there are no other things to set, or maybe as a placeholder and should only appear when nothing else is set.

When a bitflag is built with a zero valued item then it is treated as always present by bitflags functions. This may cause bugs but it definitely causes confusion when printing such a value. See the following for an example: ``` #[macro_use] extern crate bitflags; bitflags! { struct Flags: u32 { const NONE = 0b0; const SOME = 0b1; } } fn main() { let none = Flags::NONE; let some = Flags::SOME; println!("NONE = {:?}", none); println!("SOME = {:?}", some); assert!(some.contains(Flags::NONE)); } ``` Results in an output of: ``` NONE = NONE SOME = NONE | SOME ``` I noticed this when printing some flags from git2, specifically [MergeAnalysis](https://docs.rs/git2/0.7.1/git2/struct.MergeAnalysis.html). The output of a `format("{:?}", flag)` ended up being "(ANALYSIS_NONE | ANALYSIS_NORMAL | ANALYSIS_FASTFORWARD)" which confused me greatly. ANALYSIS_NONE is zero as that is how it is defined in [git2](https://github.com/libgit2/libgit2/blob/HEAD/include/git2/merge.h#L320) It seems to me that way that flag is intended to be used is that NONE only shows up when there are no other things to set, or maybe as a placeholder and should only appear when nothing else is set.

dtolnay commented

2018-04-25 21:54:11 -05:00

(Migrated from github.com)

I agree, a zero value should only show up in Debug output if no bits are set. Would you be interested in sending a PR to fix this?

Alex-Addy commented

2018-04-26 09:02:47 -05:00

(Migrated from github.com)

I would definitely be interested in doing so, where should I start?

dtolnay commented

2018-04-26 11:05:54 -05:00

(Migrated from github.com)

The Debug impls are constructed here.

The Debug impls are constructed [here](https://github.com/rust-lang-nursery/bitflags/blob/1.0.2/src/lib.rs#L401).

Alex-Addy commented

2018-04-27 08:00:44 -05:00

(Migrated from github.com)

After looking some more at the code I think this is more involved than I thought. With the code considering empty flags as always present I think that changing just the debug output will make for bugs that are even more difficult to track down.

For example some.contains(Flags::NONE) is always true in my example above, but if we remove it from the debug it will be more difficult to realise this is happening. At least none.is_empty() behaves correctly.

In any case, I am happy to add this special case if I can also add to the documentation an explanation of zero flags.

After looking some more at the code I think this is more involved than I thought. With the code considering empty flags as always present I think that changing just the debug output will make for bugs that are even more difficult to track down. For example `some.contains(Flags::NONE)` is always true in my example above, but if we remove it from the debug it will be more difficult to realise this is happening. At least `none.is_empty()` behaves correctly. In any case, I am happy to add this special case if I can also add to the documentation an explanation of zero flags.

Rows
Columns

Bitflags values of zero are considered always present #151