current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 14 down vote favorite
1

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 yesterday
add comment

5 Answers

up vote 7 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 yesterday
    
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 yesterday
    
"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 20 hours ago
add comment
up vote 8 down vote

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
up vote 7 down vote

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.

share|improve this answer
4  
This actually depends on what optimization level the OP uses. -O3 will do the job, but -O0 (the default compiler setting) will not. –  syb0rg yesterday
2  
@syb0rg Neither the OP nor anyone else has even mentioned GCC, so your comment is quite pointless... –  Lundin yesterday
2  
@Lundin GCC isn't the only compiler to implement optimization levels... in fact, most compilers offer this feature. My comment isn't irrelevant at all. –  syb0rg 22 hours ago
add comment
up vote 5 down vote

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 2 days ago
add comment
up vote 3 down vote

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 yesterday
    
Fixed, thanks. As & has precedence over |, parenthesis are optional. –  Yves Daoust yesterday
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.