Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

From most of the suggestions of answers I have on my screen, I have reduced the two functions down to:

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

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

They look so trivial now.

share|improve this question
8  
If you want bools, consider using the real bool datatype. #include <stdbool.h> –  user2357112 Mar 28 at 6:28
add comment

5 Answers 5

up vote 10 down vote accepted

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.

share|improve this answer
    
"Always use {} after each statement" --> "Always use {} after each part of a branching statement"? –  martin f Mar 29 at 0:56
    
Also, i don't understand others have commented, naming a bool "value" and using it to store values is confusing. uint8_t would have been a more suitable. I understood Jamal's advice, but not your's. –  martin f Mar 29 at 1:25
    
"Integer literals are always of the type int." I think depends on range. When int is 32 bits, 5000000000 is likely a long long and not an int. –  chux Mar 29 at 5:34
    
@martinf Regarding {}: after every if, else, else if, for, while, do-while and switch statement. Regarding the bool value: bit-wise arithmetic is usually done with integers. If you want to tell a function to set a bit to 1 or 0, it makes more sense to pass a 1 or 0 than true/false. –  Lundin Mar 31 at 6:29
1  
@chux Indeed. They can even be of unsigned type in case of hex or octal literals. –  Lundin Mar 31 at 6:42
add comment

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

share|improve this answer
add comment

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

share|improve this answer
add comment

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

share|improve this answer
1  
Maybe bitValue. –  ChrisW Mar 28 at 1:10
add comment

Here is a branchless implementation of the set_bit function:

return x & ~(1 << offset) | (value << offset);
share|improve this answer
    
You have a missing parenthesis. Should be (x & ~(1 << offset)) –  Mr.Mindor Mar 28 at 17:08
    
Fixed, thanks. As & has precedence over |, parenthesis are optional. –  Yves Daoust Mar 28 at 17:28
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.