views:

4933

answers:

14

In a C++ project I'm working on I have a flag kind of value which can have 4 values. Those 4 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 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 3 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 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.

+3  A: 

Even if you have to use 4 byte to store an enum (I'm not that familiar with C++ -- I know you can specify the underlying type in C#), it's still worth it -- use enums.

In this day and age of servers with GBs of memory, things like 4 bytes vs. 1 byte of memory at the application level in general don't matter. Of course, if in your particular situation, memory usage is that important (and you can't get C++ to use a byte to back the enum), then you can consider the 'static const' route.

At the end of the day, you have to ask yourself, is it worth the maintenance hit of using 'static const' for the 3 bytes of memory savings for your data structure?

Something else to keep in mind -- IIRC, on x86, data structures are 4-byte aligned, so unless you have a number of byte-width elements in your 'record' structure, it might not actually matter. Test and make sure it does before you make a tradeoff in maintainability for performance/space.

Jonathan
+4  A: 

Enums would be more approprite as they provide "meaning to the identifiers" as well as tape safety. You can clearly tell "xDeleted" is of "RecordType" and that represent "type of a record" (wow!) even after years. Consts would require comments for that, also they would require gouing up and down in code.

hayalci
+5  A: 

Here are couple of articles on const vs. macros vs. enums:

Symbolic Constants
Enumeration Constants vs. Constant Objects

I think you should avoid macros especially since you wrote most of your new code is in modern C++.

Abbas
+14  A: 

Have you ruled out std::bitset? Sets of flags is what it's for. Do

typedef std::bitset<4> RecordType;

then

static const RecordType xNew(1);
static const RecordType xDeleted(2);
static const RecordType xModified(4);
static const RecordType xExisting(8);

Because there are a bunch of operator overloads for bitset, you can now do

RecordType rt = whatever;      // unsigned long or RecordType expression
rt |= xNew;                    // set 
rt &= ~xDeleted;               // clear 
if ((rt & xModified) != 0) ... // test

Or something very similar to that - I'd appreciate any corrections since I haven't tested this. You can also refer to the bits by index, but it's generally best to define only one set of constants, and RecordType constants are probably more useful.

Assuming you have ruled out bitset, I vote for the enum.

I don't buy that casting the enums is a serious disadvantage - OK so it's a bit noisy, and assigning an out-of-range value to an enum is undefined behaviour so it's theoretically possible to shoot yourself in the foot on some unusual C++ implementations. But if you only do it when necessary (which is when going from int to enum iirc), it's perfectly normal code that people have seen before.

I'm dubious about any space cost of the enum, too. uint8 variables and parameters probably won't use any less stack than ints, so only storage in classes matters. There are some cases where packing multiple bytes in a struct will win (in which case you can cast enums in and out of uint8 storage), but normally padding will kill the benefit anyhow.

So the enum has no disadvantages compared with the others, and as an advantage gives you a bit of type-safety (you can't assign some random integer value without explicitly casting) and clean ways of referring to everything.

For preference I'd also put the "= 2" in the enum, by the way. It's not necessary, but a "principle of least astonishment" suggests that all 4 definitions should look the same.

Steve Jessop
Actually, I did not consider bitset at all. However, I'm not sure it would be good. With bitset, I have to address bits as 1, 2, 3, 4 which would make code less readable - meaning I would probably use an enum to 'name' the bits. Could be a space saver though. Thanks.
Milan Babuškov
Steve Jessop
Where by "etc" I mean 1, 2, 4, 8 in case that wasn't clear. bitset has an unsigned long constructor.
Steve Jessop
Milan, you don't have to "name" the bits using an enum, you can just use the predefined bits as shown above. If you want to turn on bit one, rather than my_bitset.flip(1), you would do my_bitset |= xNew;
mos
I edited my answer after Milan made his comment, to explain in detail - all I said at first was "define static const flags", which wasn't very clear.
Steve Jessop
+2  A: 

Do you actually need to pass around the flag values as a conceptual whole, or are you going to have a lot of per-flag code? Either way, I think having this as class or struct of 1-bit bitfields might actually be clearer:

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

Then your record class could have a struct RecordFlag member variable, functions can take arguments of type struct RecordFlag, etc. The compiler should pack the bitfields together, saving space.

wnoise
Sometimes as a whole, sometimes as flag. And, I also need to test if a certain flag is set (when I pass it as a whole).
Milan Babuškov
well, when separate, just ask for an int. When together, pass the struct.
wnoise
It won't be better. Access to bit fields are slower than anything else.
paercebal
Really? You think the compiler will generate significantly different code for testing bit-fields than manual bit-twiddling? And that it will be significantly slower? Why? The one thing you can't idiomatically do so easily is mask multiple flags at once.
wnoise
Running a simple reading test I get 5.50-5.58 seconds for bit-masking vs 5.45-5.59 for bit-field access. Pretty much indistinguishable.
wnoise
Wikipedia article Bit_field claims that "many popular compilers" emit bad code. In which case many popular compilers deserve a slap. And "many" on Wikipedia can mean anything from "there used to be one", to "all", since it states nothing and hence achieves consensus. So I wouldn't worry about it.
Steve Jessop
Don't use bitfields - they are almost invariably more trouble than they are worth.
Jonathan Leffler
See the Chen article referenced by @paercebal.
Jonathan Leffler
A: 

I would rather go with

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

Simply because:

  1. It is cleaner and it makes the code readable and maintainable.
  2. It logically groups the constants.
  3. Programmer's time is more important, unless your job is to save those 3 bytes.
Vivek
Well, I could easily have a million instances of the class Record, so it might be important. OTOH, that's just a difference between 1MB and 4MB, so maybe I shouldn't worry.
Milan Babuškov
+5  A: 

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

paercebal
Why do you think bitfields are slow? Have you actually profiled code using it and another method? Even if it is, clarity can be more important than speed, making "don't ever use that" a bit simplified.
wnoise
You're right. I'll edit more info.
paercebal
"static const uint8 xNew;" is only redundant because in C++ const namespace-scoped variables default to internal linkage. Remove "const" and it has external linkage. Also, "enum { ... } RecordType;" declares a global variable named "RecordType" whose type is an anonymous enum.
bk1e
I'd argue that non-thread-safety is insufficient reason to recommend never using a structure or idiom. We don't make all classes thread-safe. And when we do and get it wrong, we kind of deserve what we get (which hopefully is correction in code review)...
Steve Jessop
bk1e : Removing the "static" was already described in my post... :-) ...
paercebal
onebyone : First, the main reason was that the gain (a few bytes, if any) was overhsadowed by the loss (slower to access, both read and write)...
paercebal
onebyone : Second, All the code I produce at work or at home is inherently thread safe. It's easy to do: No globals, no static, not shared between threads unless lock-protected. Using this idiom would break this basic thread-safety. And for what? A few bytes **perhaps** ?... :-) ...
paercebal
Added reference to Raymond Chen's article on bitfields' hidden costs.
paercebal
+1 for the Raymond Chen x-ref!
Jonathan Leffler
+1  A: 

I probably wouldn't use an enum for this kind of a thing where the values can be combined together, more typically enums are mutually exclusive states.

But whichever method you use, to make it more clear that these are values which are bits which can be combined together, use this syntax for the actual values instead:

#define X_NEW      (1 << 0)
#define X_DELETED  (1 << 1)
#define X_MODIFIED (1 << 2)
#define X_EXISTING (1 << 3)

Using a left-shift there helps to indicate that each value is intended to be a single bit, it is less likely that later on someone would do something wrong like add a new value and assign it something a value of 9.

There is precedent enough for that, particularly in constants for ioctl(). I prefer to use hex constants, though: 0x01, 0x02, 0x04, 0x08, 0x10, ...
Jonathan Leffler
+2  A: 

If you want the type safety of classes, with the convenience of enumeration syntax and bit checking, consider Safe Labels in C++. I've worked with the author, and he's pretty smart.

Beware, though. In the end, this package uses templates and macros!

Don Wakefield
Looks like overkill for my little app. but it does seem like a good solution.
Milan Babuškov
+1  A: 

Based on KISS, high cohesion and low coupling, ask these questions -

  • Who needs to know? my class, my library, other classes, other libraries, 3rd parties
  • What level of abstraction do I need to provide? Does the consumer understand bit operations.
  • Will I have have to interface from VB/C# etc?

There is a great book "Large-Scale C++ Software Design", this promotes base types externally, if you can avoid another header file/interface dependancy you should try to.

titanae
a) 5-6 classes. b) only me, it's a one-man project c) no interfacing
Milan Babuškov
+1  A: 

If you are using Qt you should have a look for QFlags. The QFlags class provides a type-safe way of storing OR-combinations of enum values.

koschi
Nope, no Qt. Actually, it's a wxWidgets project.
Milan Babuškov
+1  A: 

If possible do NOT use macros. They aren't too much admired when it comes to modern C++.

Iulian Şerbănoiu
True. What I hate about macros myself is that you can't step into them if they're wrong.
carleeto
+18  A: 

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 int & 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 uses 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 arror code; e.g. when you want to return the state but the i/o operation fails. It is also useful for debugging; use it in initialisation list 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 fuction 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 0xDEADBEAF 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.

mat_geek
Mostly a nice answer - but the question stipulates that the flags can be combined and the IsValidState() function does not permit them to be combined.
Jonathan Leffler
@Jonathan Leffler: from where i stand i think the 'IsValidState' is not supposed to do that, 'IsValidMask' is.
João Portela
A: 

Not that I like to over-engineer everything but sometimes in these cases it may be worth creating a (small) class to encapsulte this information. If you create a class RecordType then it might have funtions like:

void setDeleted();

void clearDeleted();

bool isDeleted();

etc... (or whatever convention suits)

It could validate combinations (in the case where not all combintations are legal, eg if 'new' and 'deleted' could not both be set at the same time). If you just used bit masks etc then the code that sets the state needs to validate, a class can encapsulate that logic too.

The class may also give you the ability to attach meaningful logging info to each state, you could add a function to return a string representation of the current state etc (or use the streaming operators '<<').

For all that if you are worried about storage you could still have the class only have a 'char' data member, so only take a small amount of storage (assuming it is non virtual). Of course depending on the hardware etc you may have alignment issues.

You could have the actual bit values not visible to the rest of the 'world' if they are in an anonymous namespace inside the cpp file rather than in the header file.

If you find that the code using the enum/#define/ bitmask etc has a lot of 'support' code to deal with invalid combinations, logging etc then encapsulation in a class may be worth considering. Of course most times simple problems are better off with simple solutions...

Michael

Unfortunately, declaration has to be in a .h file since it is used across the project (used by some 5-6 classes).
Milan Babuškov