Enum bitfield container class

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:

Answer 1

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; }

Answer 2

*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;
}

Answer 3

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.

Answer 4

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.