Should I use #define, enum or const?


Question

In a C++ project I'm working on, I have a flag kind of value which can have four values. Those four flags can be combined. Flags describe the records in database and can be:

  • new record
  • deleted record
  • modified record
  • existing record

Now, for each record I wish to keep this attribute, so I could use an enum:

enum { xNew, xDeleted, xModified, xExisting }

However, in other places in code, I need to select which records are to be visible to the user, so I'd like to be able to pass that as a single parameter, like:

showRecords(xNew | xDeleted);

So, it seems I have three possible appoaches:

#define X_NEW      0x01
#define X_DELETED  0x02
#define X_MODIFIED 0x04
#define X_EXISTING 0x08

or

typedef enum { xNew = 1, xDeleted, xModified = 4, xExisting = 8 } RecordType;

or

namespace RecordType {
    static const uint8 xNew = 1;
    static const uint8 xDeleted = 2;
    static const uint8 xModified = 4;
    static const uint8 xExisting = 8;
}

Space requirements are important (byte vs int) but not crucial. With defines I lose type safety, and with enum I lose some space (integers) and probably have to cast when I want to do a bitwise operation. With const I think I also lose type safety since a random uint8 could get in by mistake.

Is there some other cleaner way?

If not, what would you use and why?

P.S. The rest of the code is rather clean modern C++ without #defines, and I have used namespaces and templates in few spaces, so those aren't out of question either.

1
122
7/26/2015 10:37:36 PM

Accepted Answer

Combine the strategies to reduce the disadvantages of a single approach. I work in embedded systems so the following solution is based on the fact that integer and bitwise operators are fast, low memory & low in flash usage.

Place the enum in a namespace to prevent the constants from polluting the global namespace.

namespace RecordType {

An enum declares and defines a compile time checked typed. Always use compile time type checking to make sure arguments and variables are given the correct type. There is no need for the typedef in C++.

enum TRecordType { xNew = 1, xDeleted = 2, xModified = 4, xExisting = 8,

Create another member for an invalid state. This can be useful as error code; for example, when you want to return the state but the I/O operation fails. It is also useful for debugging; use it in initialisation lists and destructors to know if the variable's value should be used.

xInvalid = 16 };

Consider that you have two purposes for this type. To track the current state of a record and to create a mask to select records in certain states. Create an inline function to test if the value of the type is valid for your purpose; as a state marker vs a state mask. This will catch bugs as the typedef is just an int and a value such as 0xDEADBEEF may be in your variable through uninitialised or mispointed variables.

inline bool IsValidState( TRecordType v) {
    switch(v) { case xNew: case xDeleted: case xModified: case xExisting: return true; }
    return false;
}

 inline bool IsValidMask( TRecordType v) {
    return v >= xNew  && v < xInvalid ;
}

Add a using directive if you want to use the type often.

using RecordType ::TRecordType ;

The value checking functions are useful in asserts to trap bad values as soon as they are used. The quicker you catch a bug when running, the less damage it can do.

Here are some examples to put it all together.

void showRecords(TRecordType mask) {
    assert(RecordType::IsValidMask(mask));
    // do stuff;
}

void wombleRecord(TRecord rec, TRecordType state) {
    assert(RecordType::IsValidState(state));
    if (RecordType ::xNew) {
    // ...
} in runtime

TRecordType updateRecord(TRecord rec, TRecordType newstate) {
    assert(RecordType::IsValidState(newstate));
    //...
    if (! access_was_successful) return RecordType ::xInvalid;
    return newstate;
}

The only way to ensure correct value safety is to use a dedicated class with operator overloads and that is left as an exercise for another reader.

86
12/18/2013 4:54:46 PM

Forget the defines

They will pollute your code.

bitfields?

struct RecordFlag {
    unsigned isnew:1, isdeleted:1, ismodified:1, isexisting:1;
};

Don't ever use that. You are more concerned with speed than with economizing 4 ints. Using bit fields is actually slower than access to any other type.

However, bit members in structs have practical drawbacks. First, the ordering of bits in memory varies from compiler to compiler. In addition, many popular compilers generate inefficient code for reading and writing bit members, and there are potentially severe thread safety issues relating to bit fields (especially on multiprocessor systems) due to the fact that most machines cannot manipulate arbitrary sets of bits in memory, but must instead load and store whole words. e.g the following would not be thread-safe, in spite of the use of a mutex

Source: http://en.wikipedia.org/wiki/Bit_field:

And if you need more reasons to not use bitfields, perhaps Raymond Chen will convince you in his The Old New Thing Post: The cost-benefit analysis of bitfields for a collection of booleans at http://blogs.msdn.com/oldnewthing/archive/2008/11/26/9143050.aspx

const int?

namespace RecordType {
    static const uint8 xNew = 1;
    static const uint8 xDeleted = 2;
    static const uint8 xModified = 4;
    static const uint8 xExisting = 8;
}

Putting them in a namespace is cool. If they are declared in your CPP or header file, their values will be inlined. You'll be able to use switch on those values, but it will slightly increase coupling.

Ah, yes: remove the static keyword. static is deprecated in C++ when used as you do, and if uint8 is a buildin type, you won't need this to declare this in an header included by multiple sources of the same module. In the end, the code should be:

namespace RecordType {
    const uint8 xNew = 1;
    const uint8 xDeleted = 2;
    const uint8 xModified = 4;
    const uint8 xExisting = 8;
}

The problem of this approach is that your code knows the value of your constants, which increases slightly the coupling.

enum

The same as const int, with a somewhat stronger typing.

typedef enum { xNew = 1, xDeleted, xModified = 4, xExisting = 8 } RecordType;

They are still polluting the global namespace, though. By the way... Remove the typedef. You're working in C++. Those typedefs of enums and structs are polluting the code more than anything else.

The result is kinda:

enum RecordType { xNew = 1, xDeleted, xModified = 4, xExisting = 8 } ;

void doSomething(RecordType p_eMyEnum)
{
   if(p_eMyEnum == xNew)
   {
       // etc.
   }
}

As you see, your enum is polluting the global namespace. If you put this enum in an namespace, you'll have something like:

namespace RecordType {
   enum Value { xNew = 1, xDeleted, xModified = 4, xExisting = 8 } ;
}

void doSomething(RecordType::Value p_eMyEnum)
{
   if(p_eMyEnum == RecordType::xNew)
   {
       // etc.
   }
}

extern const int ?

If you want to decrease coupling (i.e. being able to hide the values of the constants, and so, modify them as desired without needing a full recompilation), you can declare the ints as extern in the header, and as constant in the CPP file, as in the following example:

// Header.hpp
namespace RecordType {
    extern const uint8 xNew ;
    extern const uint8 xDeleted ;
    extern const uint8 xModified ;
    extern const uint8 xExisting ;
}

And:

// Source.hpp
namespace RecordType {
    const uint8 xNew = 1;
    const uint8 xDeleted = 2;
    const uint8 xModified = 4;
    const uint8 xExisting = 8;
}

You won't be able to use switch on those constants, though. So in the end, pick your poison... :-p


Licensed under: CC-BY-SA with attribution
Not affiliated with: Stack Overflow
Icon