Setting and getting bits in C

Question

I am just starting to get my feet wet with C; and am rather enjoying myself in doing so. This is sort of a follow up to my last question here. I am posting this ordinate to hopefully getting some feedback on the logic itself, and possibly also some feedback on coding style. I do, however insist on making everything explicit. I'm not a huge fan of implicit anything... Maybe that's something I need to get over?

#include <stdio.h>

typedef enum {
    false = (int) 0, true = (int) 1
} bool;

inline void set_bit(register unsigned int *x,
        register const unsigned short int offset, register const bool value) {
    if (value)
        *x |= (1 << offset);
    else
        *x &= ~(1 << offset);
}

inline bool get_bit(register const unsigned int x,
        register const unsigned short int offset) {
    return (x & (1 << offset)) != 0;
}

int main(void) {
    unsigned int x = 0;
    int count;
    for (count = 0; count < 8; count += 2)
        set_bit(&x, count, true);
    for (count = 7; count >= 0; count--)
        printf("%d ", (int) get_bit(x, count));
    printf("\n");
    return 0;
}

Answer 1

I do, however insist on making everything explicit. I'm not a huge fan of implicit anything... Maybe that's something I need to get over?

No, that is very good practice and a good habit. Particularly, there are many dangerous, implicit type promotions going on in C, that explicit type casts can prevent.

However, if you want to be explicit, you must actually have quite a deep understanding of what's going on between the C lines. Until you do, you will end up causing more problems than you solve with this coding practice. Also, you do a couple of pointless things that only clutter down the code.

---

Here are my comments on your code:

• Do not define your own bool type. Instead use the standard bool type in stdint.h.
• (int) 0 This cast is pointless and adds nothing of value. Integer literals are always of the type int. Enumeration constants are also always of the type int.
• register is an obsolete keyword from the dark ages, when compilers were worthless at optimizing code. Today, a modern compiler does a much better job at optimizing the code than you do, so let it do its job. Also, the register keyword causes various side-effects: for example you can't take the address of a register variable. So never use register, because there is never a reason to do so. That keyword just remains in the language for backwards compatibility.
• Similarly, it is often best to let the compiler handle inlining in most cases. Because while inlining makes the code faster, it also makes the executable much larger. The compiler is likely better than you at determining when to optimize for size and when to optimize for speed.
• Always use {} after each statement, even if there is only one line of code following it. Skipping {} is dangerous practice that almost certainly leads to bugs sooner or later.

• You want to be explicit but miss a few cases where it actually matters.

  • 1 << offset means that you do left shift on a signed integer. Assuming 32 bit integers, the case of 1 << 31 leads to undefined behavior. Generally, in 99.9% of the cases it doesn't make any sense whatsoever to use bitwise operators on signed integers. If you wish to write safe code, be explicit and type 1u << offset.

  • Because of the above, *x |= (1 << offset); implicitly stores an int into an unsigned int, which is potentially dangerous in some cases.

  • And also because of the above, you apply ~ to a signed int, which is always dangerous practice.

  • You have no function declarations, only definitions. You also don't declare the local functions as static, which is proper practice for functions that are private to the specific module.

• As others have commented, naming a bool "value" and using it to store values is confusing. uint8_t would have been a more suitable type to use in this case.

• In general, it is always better to use the types from stdint.h when you do bit manipulations.

Answer 2

I think set_bit might be better as:

unsigned set_bit(unsigned x, unsigned offset, bool value)
{
    return (value)
        ? x | (1 << offset)
        : x & ~(1 << offset);
}

I'm not sure why I think so; my reasons are:

• It's closer to the macros I used to see for doing this kind of thing
• Pointers can be abused (e.g. you could pass a null pointer into your version)
• The compiler might find it easier to optimize (e.g. because taking the address of a local variable and passing it to a subroutine might make the compiler worry about aliasing).

---

You might get better performance by defining separate functions named set_bit and clear_bit (OTOH your compiler might optimize away the difference when you pass a hard-coded value to your set_bit).

---

I don't think that short adds any value to the offset parameter; shorts are often less efficient performance-wise; and even a short value is too long for your needs (you probably want offset to have a value from 0 through 31 or 63).

Answer 3

Drop the inline and register keywords. The compiler will decide what to do better than you can, despite your personal preference for complete mastery over the machine.

Keep in mind that the default compiler optimization setting is set at -O0. For this answer to work properly, you should also set your minimum compiler optimization level to -O3 (if you haven't already).

Answer 4

value is not a useful name for a bool variable; it sounds more like it's holding a number rather than a condition. Do you mean for it to be something like isBitValue?

If by explicit, you mean defining your own bool type, then that may be okay for a toy program in C. Otherwise, you should use <stdbool.h>.

Answer 5

Here is a branchless implementation of the set_bit function:

return x & ~(1 << offset) | (value << offset);