Enum bitfield container class
-
22-08-2019 - |
Question
Im trying to write a small class to better understand bit flags in c++. But something isnt working out. It prints the wrong values. Where is the problem? Have I misunderstood how to add flags? Or check if the bit field has them?
Heres the code:
#include <iostream>
enum flag
{
A = 1, B = 2, C = 4
};
class Holder
{
public:
Holder() : m_flags(A) {}
~Holder() {}
void add_flag(flag f) { m_flags |= f; }
bool has_flag(flag f) { return ((m_flags&f)==f); }
void remove_flag(flag f)
{
unsigned int flags = 0;
for (int i = 1; i<=(int)C; i *= 2)
{
if ((flag)i!=f && has_flag(f))
flags |= f;
}
m_flags = flags;
}
void print()
{
std::cout << "flags are now: " << m_flags << " | holding: ";
for (int i = 1; i<=(int)C; i *= 2)
{
if (has_flag((flag)i))
std::cout << i << " ";
}
std::cout << std::endl;
}
private:
unsigned int m_flags;
};
int main()
{
Holder h;
h.print(); // should print 1
h.add_flag(B);
h.print(); // should print 1 2
h.remove_flag(A);
h.print(); // should print 2
h.add_flag(C);
h.print(); // should print 2 4
h.remove_flag(B);
h.print(); // should print 4
}
Output of program:
flags are now: 1 | holding: 1
flags are now: 3 | holding: 1 2
flags are now: 1 | holding: 1
flags are now: 5 | holding: 1 4
flags are now: 0 | holding:
Solution
personally I would use std::vector< bool > to handle flags, since it is a specialization that packs bools into bit.
However:
I think your remove flag is a bit complex, try this instead
void remove_flag( flag f )
{
if ( has_flag( f ) == true )
{
m_flags ^= f; // toggle the bit leaving all other unchanged
}
}
Edit:
A comment asked why I just didn't do m_flags &= ~f
. I took the question as a 'learner' question not an optimization question. I show how to make his code correct, not fast.
OTHER TIPS
There's a bug in your remove_flag() method, it should be flags |= i;
But, do it O(1) like this:
void remove_flag(flag f) { m_flags &= ~f; }
has_flag() and remove_flag() are wrong. They should go like this:
bool has_flag(flag f) { return !!(m_flags & f); }
void remove_flag(flag f)
{
m_flags &= ~f;
}
Everyone has already nailed this: flag &= ~f;
You might look at my earlier posting.
has_flag(): Are you looking to return true if all the bits in f are set? Or if at least one of them is set? It's the difference between flags&f==f vs flags&f !=0.
You might consider #include <iomanip> and cout << hex <<m_flag <<dec. (Hex to bit conversion can be more easily done in your head.)
The enum could be inside class Holder.
class Holder
{
public:
enum flag { A=1, B=2, C=4; };
...
};
You would then use Holder::A instead of A.
You may want to use for(i=0;i<N;i++) if has_flag(1<<i) ...
You may want your add_flag/has_flag/remove_flag methods to take an int rather than the enumerated type. That gets rid of a lot of casting. If you don't want to support all possible int values, a validation method and rejection path could be used. By the way, there's nothing stopping me from calling add_flag(flag(5736)). And you are casting to enum_flag quite frequently already.
You may want to use mFlag rather than m_flag. It's your choice. But when you look at code like m_x*m_y-m_z*m_y-m_x*m_z , depending on your font, it can be easy to mistake _ for -. (Or vice versa.)
Similarly, consider addFlag rather than add_flag. For something like this it doesn't matter. But when you have a long descriptive name, those underscores start to add up, using up linespace. The temptation is then to abbreviate the name, making your code more obtuse.
Just my $0.02.