HiveBrain v1.2.0
Get Started
← Back to all entries
patterncModerate

Setting and getting bits in C

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
andbitssettinggetting

Problem

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 

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 = 0; count--)
        printf("%d ", (int) get_bit(x, count));
    printf("\n");
    return 0;
}

Solution

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

-
Because of the above,
*x |= (1

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

Context

StackExchange Code Review Q#45551, answer score: 13

Revisions (0)

No revisions yet.