tags:

views:

274

answers:

6

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

+7  A: 

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.

anon
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
@James You mean that BIT0 might actually be defined as (1 << 31) - oh, that's sick! But possible I guess.
anon
@Neil: Well, I was more thinking of starting counting with the lowest order bit, but I guess either way would work.
James McNellis
@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?
anon
@Neil: BIT0 = 0x00000001 and BIT1 = 0x00000002 etc
0A0D
@Changeling Then if these values will never change, that seems a bit (no pun intended) pointless - use integer literals directly.
anon
@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 - Matthew Orlando
@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.
anon
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
@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 - Matthew Orlando
@Cogwheel Because generally, you can't.
anon
@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
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
@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 - Matthew Orlando
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
@Mike Please indicate who you are talking to via the @xxx mechanism.
anon
@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.
0A0D
+3  A: 

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 - Matthew Orlando
should this be `template<typename T1, typename T2>` instead?
0A0D
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 - Matthew Orlando
In your example, I get `Could not find a match for setBit<T>(unsigned int,SystemState)`
0A0D
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 - Matthew Orlando
This seems to work if both types are the same. Apparently the macro never checked that
0A0D
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
@Johannes: Yes. There is a lot of casting to do now :)
0A0D
I can't see any reason for using templates at all here.
anon
@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 - Matthew Orlando
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 - Matthew Orlando
Anyone care to comment on the down vote?
Cogwheel - Matthew Orlando
@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),
anon
@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
: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.
anon
@Neil srsly i envy you for having done more downvotes than upvotes xD
Johannes Schaub - litb
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 - Matthew Orlando
@Johannes There are a lot of things that need downvoting.
anon
@Cogwheel You should never make anything more general than necessary - basic rule of software development.
anon
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 - Matthew Orlando
@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
@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.
anon
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 - Matthew Orlando
@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 - Matthew Orlando
anon
@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 - Matthew Orlando
@Cogwheel I would replace them (if they needed replacing, which isn't obvious) with (maybe inline) functions, with parameters of the correct type.
anon
@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.
anon
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 - Matthew Orlando
@Neil: If I specify the type, wouldn't that be more specific than say using a macro, which has no type safety?
0A0D
@Cogwheel No, it wasn't. I simply thought of some more reasons why your answer is bad.
anon
@Changeling Specifying the type is good - and a normal function is more type-safe than a template.
anon
@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 - Matthew Orlando
@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".
anon
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 - Matthew Orlando
@Cogwheel One of us had better put this to an end - goodbye.
anon
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 - Matthew Orlando
@Neil and @Cogwheel: Thank you both for your insight.@Neil: Can you post an inline function instead versus inline template?
0A0D
@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 - Matthew Orlando
@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.
0A0D
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 - Matthew Orlando
@Cogwheel: Well I tried that and found that I have int,SystemState and char,SystemState and unsigned int,SystemState.. even with doing `template<typename T1, typename T2>` 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?
0A0D
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 - Matthew Orlando
@Cogwheel: Sorry, no compute :) I had the second one as the SystemState, are you saying reverse word and mask?
0A0D
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 - Matthew Orlando
@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
0A0D
+6  A: 

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
+1: Indeed, setting flags like this is common and acceptable C++. Just look at `iostream` flags, for example.
Justin Ardini
+2  A: 

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
+1  A: 

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
+1  A: 

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