Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

The comment at the top was not followed because I printed the contents. I just want an opinion on coding style.

    /*
     * Write a function setbits(x,p,n,y) that returns x with the n bits
     * that begin at position p set to the rightmost n bits of y, leaving
     * the other bits unchanged in the least number of lines.
     *
     * Note: function signatures and curly brackets dont count towards 
     * the number of lines. You must also declare your variables, and 
     * call the function
     *
     * build with:
     *   gcc -o bit_twiddle -Wall -g -lm ./bit_twiddle.c
     */

    #include <stdio.h>
    #include <math.h>
    #include <limits.h>

    unsigned setbits(unsigned x, unsigned p, unsigned n, unsigned y) {
        x |= (y & ~(~0 << n)) << p;
        size_t s = (int)(log(INT_MAX)/log(2)) + 1;
        printf("An int is %d bits\n", s);
        int mask = pow(2.0, (int)s);
        do {
            ((x & mask) == 0) ? printf("0") : printf("1");
            ((s%4)==0) ? printf(" ") : printf("");
            x <<= 1;
        } while (s--);
        printf("\n");
    }

    void main( ) {
        unsigned retrn=1, begin=3, nbits=3, input=7;
        unsigned x = setbits(retrn, begin, nbits, input);
    }

UPDATE

x |= (y & ~(0 << n)) << p --> x |= (y & ~(~0 << n)) << p
share|improve this question
1  
Not sure this does anything: (0 << n) –  Loki Astari Nov 16 '11 at 2:59

1 Answer 1

up vote 6 down vote accepted

I think your code does not exactly what the comment wants. In line:

x |= (y & ~(0 << n)) << p;

0 << n is 0. If you want n rightmost bits of y you may use a mask like (1<<n)-1 (which is n 1 bits). Now on your code style, when you are coding a bit manipulating task, you'd better do it completely with bitwise operators (at least use other functions very few). Here in this code, using log and pow can be avoided. If you want to get number of bits in an integer type, you can use sizeof, like below:

size_t s = sizeof(int) << 3;

here << 3 is equal to * 8, because each byte definitely has 8 bits (it works on integer types of any size, just replace int with any other type like short or long long).

Now instead of using pow(2, s) you can write 1 << s :)

And about your printing, you can replace s%4 with s&3 (think about it). And I prefer shifting down mask instead of shifting up x (here it doesn't make any difference, but when you work with signed values it does, because sign will be preserved in left shifts). And for reducing number of lines for printing, you may use for loop.

share|improve this answer

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.