Enum vs Macro States C++

(Question is related to my previous questions here, here, here, and here).

I am maintaining a very old appli开发者_StackOverflowcation that was ported years ago from DOS to Windows, but a lot of the old C conventions still carry forward.

The one particular convention is a setBit and clrBit macro:

#ifndef setBit
#define setBit(word, mask) word |= mask
#endif

#ifndef clrBit
#define clrBit(word, mask) word &= ~mask
#endif

I found that I could declare a variable as an enum type and set my variable equal to the one of the enumerated values that are defined.

enum SystemStatus
{
    SYSTEM_ONLINE                = BIT0,
    SYSTEM_STATUS2               = BIT1,
    SYSTEM_STATUS3               = BIT2,
    SYSTEM_STATUS4               = BIT3
};

With BIT0 = 0x00000001, BIT1 = 0x00000002, etc.

SystemStatus systemStatus;

systemStatus = SYSTEM_ONLINE

In your opinion, is using the setBit and clrBit macros more C like or C++ like - and would it be better to simply declare variables as an enumerated type and get rid of all the old setBit/clrBit stuff?

No you can't - assigning the enum value overwrites the whole value, while the macros are changing bits in the existing value. And what are BIT0, BIT1 et al? That's like defining INT0, INT1 etc. - terrible practice.

Bottom line, is the old C-style code giving you any problems? If not, apply that time-honoured rule - if it ain't broke, don't fix it.

setBit & clrBit are fine, although I would convert them into inline functions in C++. They would be very handy if the status bits are independant of each other, such that:

  SystemStatus systemStatus = SYSTEM_ONLINE | SYSTEM_STATUS3;

is a valid setting.

systemStatus = clrBit(systemStatus, SYSTEM_STATUS3);
systemStatus = setBit(systemStatus, SYSTEM_STATUS4);

I think you're confusing the purposes. The enum is about setting up values to use as flags. setBit and clrBit are about operating on data. That data might happen to be a flag, but that's really the only relationship between the two ideas.

That being said, macros are certainly NOT the C++ way of doing things. You would use inline functions instead. For example:

template<typename T>
inline T& setBit(T& word, T mask) { return word |= mask; }

template<typename T>
inline T& clrBit(T& word, T mask) { return word &= ~mask; }

Edit to fend off nitpickers:

This is just an example of how you could implement the functions. You don't need to use templates, you can use two template parameters instead of 1, you can use a void function or values instead of references if you want, (though then it would lose some of the original semantics). The main point is to get the benefits of type safety which you won't find in macros (among many other downsides of macros). http://www.parashift.com/c++-faq-lite/inline-functions.html#faq-9.5

Edit: Here's a void, non-template version for comparison

inline void setBit(unsigned int word, unsigned int mask) { word |= mask; }

inline void clrBit(unsigned int word, unsigned int mask) { word &= ~mask; }

If you know for sure that in all the combinations and permutations of that code, people only ever use one bit at a time, that they clear before they set and never set twice in a row, then sure, use the enum instead. It will be clearer and more readable. But if sometimes the system is 0101 then your enum can't handle that.

OK, if the enums are bitflags so you might write

systemStatus = SYSTEM_ONLINE | BIT2;

Then I guess that is readable and can support the combinations, ok.

Using an enum as you have done only works if you can ensure that no more than one bit should be set at a time. Otherwise you have to have an enumerated constant for all bit combinations, which can quickly become complex fairly quickly. You can use a set of #define statements (or an enum, I suppose) to alias bitmasks with a friendly name, but you will still likely end up using set/clear macros.

Setting and clearing bits definitely seems more of a "C-like" approach. However, I (personally) don't consider your enum approach very "C++-like". For a more C++-like approach, create a class to represent the system status and manipulate class members instead of bit fields. You could even overload the + and - operators to act like your setBit and clrBit function, respectively. For example, using systemStatus -= SYSTEM_ONLINE (with #define SYSTEM_ONLINE (1<<31) or similar) to clear the "System Online" bit if and only if it is set. You could possibly even inherit from a STL Bitset and re-use most of that functionality.

Edit: OP clarified that BIT0, etc are bitmasks, so my first paragraph is no longer relevant.

I agree with bta about if you want use a C++ approach you should create a class that abstract all implementation about the states.

But I wont overload the +=, -= operators because you continue carrying C old school.

I suggest the declaration of method to do this.

You could choice between one method with a boolean flag or two for setup & clear.

class Status{...};

void main(){
    Status status;

    //first approach
    status.SystemOnline(true);
    status.BackUPOnline(false);

    //second approach
    status.SetEmergencySystem();
    status.ClearSystemOnline();
}

with this style you encapsulate the implementation about how to implement the storage of the information.