3

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

I am maintaining a very old application 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?

Community
  • 1
  • 1
  • As Neil points out, they aren't exactly the same. That said, in C++ you should never use macros as functions, use a template function instead. Furthermore, for a collection of bits just use `std::bitset` and be done with it. (Much nicer interface.) – GManNickG Jul 13 '10 at 19:34
  • In order to enumerate all possible states this way, you need 2^(number of bits) states. Probably not the cleanest solution. – Justin Ardini Jul 13 '10 at 19:43
  • 4
    @Justin Such is the nature of bits. –  Jul 13 '10 at 19:57
  • What Changeling seems to be describing is a "one-hot" state machine, where each bit represents one state and only one bit is allowed to be set at any time (all other combinations are illegal and would be caught by a `default` clause in the `switch` block). However, this notation is rarely seen outside firmware, since its main goal is to reduce combinational logic complexity by adding registers, and doesn't really make a bit of difference in a software-only state machine. – Mike DeSimone Jul 13 '10 at 20:06

6 Answers6

7

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.

  • 3
    Unless they are constants for the zeroth, first, second, etc. order bits. That's not _that_ bad. IMO it's easier to read than `0x00004000`. – James McNellis Jul 13 '10 at 19:35
  • 1
    @James You mean that BIT0 might actually be defined as (1 << 31) - oh, that's sick! But possible I guess. –  Jul 13 '10 at 19:37
  • @Neil: Well, I was more thinking of starting counting with the lowest order bit, but I guess either way would work. – James McNellis Jul 13 '10 at 19:39
  • @James Either way I would have thought implies conditional compilation for different architectures, in which case I'd rather wrap the whole enum in an #ifdef block and use integer literal constants in each. But perhaps the OP can tell us how BIT0 et al are actually defined? –  Jul 13 '10 at 19:43
  • @Neil: BIT0 = 0x00000001 and BIT1 = 0x00000002 etc –  Jul 13 '10 at 19:49
  • @Changeling Then if these values will never change, that seems a bit (no pun intended) pointless - use integer literals directly. –  Jul 13 '10 at 19:55
  • @Neil: the discussion about the BIT values was its own question: http://stackoverflow.com/questions/3199761/define-bit0-bit1-bit2-etc-without-define -- FWIW, I agree it's a tad pointless for anyone who is comfortable with C and other low level programming, but there is a lot of ignorance about bit manipulation in the general public. It's not necessarily immediately obvious (though it should be) that 0x01, 0x02, 0x04, etc are all simply different bit positions. I'm all for added clarity, even if it's a bit frivolous. – Cogwheel Jul 13 '10 at 20:43
  • @Cogwheel Aargh! This is not code written by the general public, this is code written for and by C programmers! All defines like this do is pile obfuscation and the possibility for error on what would actually be clearer code if "magic numbers" were used instead. Anyone that writes stuff like this needs taking out and giving a good kicking. –  Jul 13 '10 at 20:53
  • 2
    Instead of `BIT0`, `BIT1`, etc., common practice (in C in embedded systems, at least) is to use a macro: `#define BIT(n) (1 << (n))`. Then another set of `#define` macros would assign bit values to field names, e.g. `#define SPI_START BIT(0)`, where these field names are as close as possible to those in the chip's data sheet. Also, `setBit` would be better named as `setBits`, since you can set several bits at once (e.g. `setBits(spiCtrlReg, SPI_START | SPI_CONT);`. – Mike DeSimone Jul 13 '10 at 22:17
  • @Mike DeSimone: Yep. that was brought up in the other question. I'm sure @Neil would have something vitriolic to say about it though. Heaven forbid you generalize the creation of bit values. – Cogwheel Jul 13 '10 at 22:22
  • @Cogwheel Because generally, you can't. –  Jul 13 '10 at 22:26
  • @Cogwheel: I have yet to discover a technique that does a perfect job of coding bitfield-register-access semantics into a C++ class or template. I have seen many things get close, but they usually result in things like broken inlining or RAM wasted on a pointer to a hardware register instead of that register's address getting compiled into the code as a constant. (This is really important for us embedded folks who are running in 1k of RAM and 32k of flash, and completely opposite heavy-iron programs where everything is in RAM.) But there's *always something* that breaks generalization. – Mike DeSimone Jul 13 '10 at 22:56
  • Sometime in the past year or two, there were a series of columns in [Embedded System Design](http://www.eetimes.com/design/embedded) on pretty much this topic, but I'm having a devil of a time finding them. I think they were written by Jack Ganssle. – Mike DeSimone Jul 13 '10 at 22:58
  • @Mike: I didn't mean templates, I just meant generalizing the idea of getting the value of a particular bit position for constants. Neil seems to have a thing against generalization. Really I shouldn't have said anything... We were in the midst of a heated debate in my answer's comments. Sorry for the trouble. – Cogwheel Jul 13 '10 at 23:05
  • Well, the "generalization" here is really just there to pretty up the code and make it more legible without having to type out 32 or 64 `BITxx` definitions. Since the ultimate goal is to declare symbols that match (as close as possible) the ones in the hardware data sheet, the "generalization" goes *poof* once those symbols are defined. – Mike DeSimone Jul 13 '10 at 23:15
  • @Mike Please indicate who you are talking to via the @xxx mechanism. –  Jul 13 '10 at 23:19
  • @Neil: Can you post an example doing this with an inline function? The function would have to be global to work in my case and not belong to a Class. –  Jul 13 '10 at 23:29
6

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);
James Curran
  • 101,701
  • 37
  • 181
  • 258
4

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; }
Cogwheel
  • 22,781
  • 4
  • 49
  • 67
  • should this be `template` instead? –  Jul 13 '10 at 19:56
  • It's probably better for the types to match so that you're guaranteed to have the same bit width for the value and the mask. You can use conversions at the call site to take care of corner cases. Of course, you could use two different types if it would better suit your purposes. – Cogwheel Jul 13 '10 at 19:58
  • In your example, I get `Could not find a match for setBit(unsigned int,SystemState)` –  Jul 13 '10 at 20:01
  • 1
    Hence the last two sentences of my comment ;). You either need to cast the SystemState argument to unsigned int, or you can use the two type version. I prefer the former, but mostly because I have a great respect for languages that don't do any automatic conversions. – Cogwheel Jul 13 '10 at 20:30
  • This seems to work if both types are the same. Apparently the macro never checked that –  Jul 13 '10 at 20:32
  • It's easy to pass different typed arguments here, which makes the templates difficult to use. For example, you may expect `unsigned int flag = ...; clrBit(flag, 0);` to work, but with a single parameter it won't. – Johannes Schaub - litb Jul 13 '10 at 20:52
  • @Johannes: Yes. There is a lot of casting to do now :) –  Jul 13 '10 at 20:56
  • I can't see any reason for using templates at all here. –  Jul 13 '10 at 21:04
  • @Neil Butterworth: hasty generalization (pun!). Seriously, though, I'm a bit biased having done some Functional Programming recently. I like to use templates for pretty much anything that might be applicable to more than one type. – Cogwheel Jul 13 '10 at 21:07
  • FWIW, I changed them to return the result so it behaves more like the macros. You still might run into some problems if the macros were used in "abusive" ways. The macros could use some extra parentheses to begin with... – Cogwheel Jul 13 '10 at 21:09
  • @Gogwheel In this case, the macros are not really applicable to more than one type, and so should be translated to normal functions (if anything), –  Jul 13 '10 at 21:15
  • @Neil some may say "oh, i can pass std::bitset<> now!" :) Of course, with this template that has only one parameter, all these sweet possibilities are thrown away :( – Johannes Schaub - litb Jul 13 '10 at 21:16
  • :Cogwheel Yes, that was me. I broke my personal rule about not downvoting answers on questions I am also answering, because I think the use of templates here is wrong. In fact, the OP's code does not need changing at all. Macros are part of C++ (I use them) and always will be. –  Jul 13 '10 at 21:17
  • @Neil srsly i envy you for having done more downvotes than upvotes xD – Johannes Schaub - litb Jul 13 '10 at 21:18
  • I don't understand why it's *wrong* to make something more general than "necessary", and I don't understand why you hold such a narrow definition of "necessary". By using the template (just a few extra keystrokes), it automatically (and safely) supports data of different widths. If you know for certain you are only ever dealing with unsigned int and equivalent, then feel free to use an int function. But how is making something more useful a bad idea and worthy of a down vote? – Cogwheel Jul 13 '10 at 21:19
  • 1
    @Johannes There are a lot of things that need downvoting. –  Jul 13 '10 at 21:20
  • @Cogwheel You should never make anything more general than necessary - basic rule of software development. –  Jul 13 '10 at 21:21
  • Tell that to the FP community... But I digress. I can accept we have different views about what's best here, I just don't see this as being so wrong. – Cogwheel Jul 13 '10 at 21:23
  • @Cogwheel the FP community though usually produces general solutions where they are necessary. `boost::function`, `boost::variant`, etc. Of course there are always people that wanna put it beyond reason, without doubt. Personally, i don't like overgeneralization either, but i'm not particularly opposed to your template (if it would have two parameters, i would even enjoy it). – Johannes Schaub - litb Jul 13 '10 at 21:24
  • @Cogwheel I suppose I've been using FP on and off for the past 25 years (first wrote a LISP program in 1984) and more recently played with Scheme and Haskell, but I never realised that writing functions to be more general than necessary was part of FP. Oh well. –  Jul 13 '10 at 21:28
  • I guess that's the real point of the issue. I agree that *over*generalization is a bad idea, but I just don't see this as over-generalization. Regarding FP, Haskell, for example, uses generic types by default. It's not default in C++, and in many cases the overhead of templates can be unwieldy. I just disagree completely that that's the case here. I also don't feel like I should have every possible variation on the basic theme in my answer just to satisfy some picked nits. You could write the function any way you want. template, no template, 2 parameters, whatever. I only was giving an example – Cogwheel Jul 13 '10 at 21:29
  • @Neil: (my last comment was in response to litb): You've recently played with Haskell, so have I. Haskell uses generic types by default. Every function is more general than "necessary" unless you specify otherwise. – Cogwheel Jul 13 '10 at 21:46
  • 1
    @Cogwheel Given that op != and op &= can be overloaded for any types, you have the possibility that setBit and clrBit can be called on types which have nothing whatsoever to do with bits. I don't think this is a good idea. In fact when it comes to bit manipulation in C++ it is normally a good idea to be extremely specific about the types being manipulated, one might even say it is necessary to do so.. –  Jul 13 '10 at 21:54
  • @Neil: The original macros suffer the same problem with the added bonus of no type safety. How is my answer worse for that fact? – Cogwheel Jul 13 '10 at 21:55
  • @Cogwheel I would replace them (if they needed replacing, which isn't obvious) with (maybe inline) functions, with parameters of the correct type. –  Jul 13 '10 at 22:01
  • @Cogwheel also note that your use of references may make the functions far less efficient on the OP's integer types. This may or may not be important of course, but it's another example of where unnecessary "generality" may be damaging. –  Jul 13 '10 at 22:15
  • So in other words it was a subjective nit pick of a down vote, which has since been addressed by my edit. I can live with that... – Cogwheel Jul 13 '10 at 22:18
  • 1
    @Neil: If I specify the type, wouldn't that be more specific than say using a macro, which has no type safety? –  Jul 13 '10 at 22:20
  • @Cogwheel No, it wasn't. I simply thought of some more reasons why your answer is bad. –  Jul 13 '10 at 22:21
  • 1
    @Changeling Specifying the type is good - and a normal function is more type-safe than a template. –  Jul 13 '10 at 22:24
  • 1
    @Neil, you're being incredibly one-sided. For every "reason" you say my answer is "bad", there are also reasons that macros are bad. You also seem to still miss the point in my edit. There are many ways to solve any particular problem. I posted one as an example. You obviously agree that inline functions are better than macros, given the nature of some of your criticisms of my answer. That was my only point from the very beginning. The fact that I used a template is simply personal preference. You know, the same reason why you wouldn't. – Cogwheel Jul 13 '10 at 22:37
  • @Cogwheel And my first comment, which YOU decided to argue against was simply "I can't see any reason for using templates at all here". –  Jul 13 '10 at 22:41
  • Umm... how is explaining why i used the template arguing? Did you interpret "hasty generalization" as an accusation against you? I meant it as an answer to your comment... I was "hastily generalizing" (in other words, in effect I agreed with some of your later points). If that misunderstanding is what started your hostility then I'm sorry I wasn't more clear. – Cogwheel Jul 13 '10 at 22:43
  • Goodbye. If I'm right about where this hostility began, I'm willing to pop my stack of everything that happened since then. Catch you on the flip side! – Cogwheel Jul 13 '10 at 22:53
  • @Neil and @Cogwheel: Thank you both for your insight. @Neil: Can you post an inline function instead versus inline template? –  Jul 13 '10 at 23:28
  • @Changeling: see my edit. The only difference is the removal of the template header and changing the Ts where necessary. (I also changed it semantically so that it addresses some of the alternatives in my answer). – Cogwheel Jul 13 '10 at 23:36
  • @Cogwheel: In cases where we are setting the bit to a char, would this have the same results despite the cast? I mean, as long as the char is between 0 and 255. –  Jul 14 '10 at 00:33
  • It should work fine. Though if you're using these functions on more than one type, you can always use a template (with two parameters, perhaps?) to eliminate the need for explicit conversion. – Cogwheel Jul 14 '10 at 01:25
  • @Cogwheel: Well I tried that and found that I have int,SystemState and char,SystemState and unsigned int,SystemState.. even with doing `template` it complained it couldn't find a match until I did the explicit cast. I'm no pro at templates so maybe I did something wrong? –  Jul 14 '10 at 01:33
  • Hmmm... maybe the enum values are int and the mask is unsigned int, so it's complaining about the SystemState and not the char. Try making one that uses SystemState as the second parameter type. – Cogwheel Jul 14 '10 at 01:51
  • @Cogwheel: Sorry, no compute :) I had the second one as the SystemState, are you saying reverse word and mask? –  Jul 14 '10 at 02:10
  • hmm no... without seeing the code it's kind of hard to judge. What did the call site and function signatures look like exactly? – Cogwheel Jul 14 '10 at 02:27
  • @Cogwheel: `setBit(Main->mdvTestStatus, EL_TEST_STATUS);` and `mdvTestStatus` is `unsigned short` while EL_TEST_STATUS is part of an `enum` called `Bits` using your original inline template –  Jul 14 '10 at 12:36
2

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.

Kate Gregory
  • 18,808
  • 8
  • 56
  • 85
1

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.

bta
  • 43,959
  • 6
  • 69
  • 99
1

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.

Gustavo V
  • 152
  • 1
  • 1
  • 4