Bit setting and code readability

Question

I have an Arduino application (well actually a library) that has a number of status flags in it - and originally I simply declared them as ints (well uint8_t so 8 bit unsigned chars in this case). But I could have combined them all into one integer and used bitmask operations to set and test the status.

An example of the former:

if (_shift == HIGH)
{
    _shift = LOW;
}
else
{
    _shift = HIGH;
}

an example of the latter

#define SHIFT_BIT 0

if (bitRead(_flags, SHIFT_BIT) == HIGH)
{
   bitWrite(_flags, SHIFT_BIT, LOW);
}
else
{
   bitWrite(_flags, SHIFT_BIT, HIGH);
}

The former reads better, but the latter is more efficient (space and time). Should the space and time efficiency always win in this situation or is this a kind of optimization that should only happen when needed?

(Added)

For completeness here's the Wiring definition of those bitWrite etc. macros:

#define bitRead(value, bit) (((value) >> (bit)) & 0x01)
#define bitSet(value, bit) ((value) |= (1UL << (bit)))
#define bitClear(value, bit) ((value) &= ~(1UL << (bit)))
#define bitWrite(value, bit, bitvalue) (bitvalue ? bitSet(value, bit) : bitClear(value, bit))

Answer 1

Check out Raymond Chen's excellent take on this issue. In summary, you need to do some detailed calculation to find out whether the latter case is actually more efficient, depending on how many objects there are vs. how many callsites actually set these states.

As far as readability, it looks like you're doing this with member variables, which means you've probably encapsulated them in nice functions. In that case, I'm not as concerned with readability because at least the code for the people using the class looks nice. However, you could always encapsulate it in private functions if it's a concern.

Answer 2

Depending on the compliance of the AVR-GCC compiler, which I'm unsure about, you can do something like this and keep things nice and clean.

struct flags {
    unsigned int flag1 : 1;  //1 sets the length of the field in bits
    unsigned int flag2 : 4;
}; 

flags data;

data.flag1 = 0;
data.flag2 = 12;

if (data.flag1 == 1)
{
    data.flag1 = 0;
}
else
{
    data.flag1 = 1;
}

If you also want access to the entire flag int at once, then:

union 
{
    struct {
        unsigned int flag1 : 1;  //1 sets the length of the field in bits
        unsigned int flag2 : 4;
    } bits;
    unsigned int val;
} flags;

You can then access a bit with 2 levels of indirection: variable.bits.flag1<--returns single bit flag or with a single level to get the entire int worth of flags: variable.val <--returns int