CODEWITHSUNDEEP
cloopswhile-loop
Oladunni Abiodun
Oladunni Abiodun

Reputation: 215

Changing a variable in a function has no effect

unsigned char xor4(unsigned char c1, unsigned char c2){
    int i = 0;
    while(i < 8){
        if((getBit(c1, i) ^ getBit(c2, i)))
             setBit(c1,i);
        else clearBit(c1, i);
        i+=2;
    }

    return c1;
}

The above code is supposed to be a very simple function to set every other bit in a character based on the result of an xor with every other bit in a second character. For some reason, this simply doesn't work. The program seems to just ignore my while loop and return the original function.

Oh and here are my getBit, setBit and clearBit functions.

unsigned char getBit(unsigned char c, int n){
  return (c & 1<<n) >> n;
}

unsigned char clearBit(unsigned char c, int n){
  c = c & (~(1<<n));
}

unsigned char setBit(unsigned char c, int n){
  `c = c | (1<<n);
}

Upvotes: 1

Views: 500

Answers (2)

Jim Balter
Jim Balter

Reputation: 16406

You didn't post your setbit and clearbit functions, but whatever they do, that can't change c1 in the calling function. You need to either pass the address of c1 or return the new value of c1. Or you can just use C bitwise operators. And your entire operation is just equivalent to c1 ^= c2 (assuming that a char has 8 bits).

Edit: Given your edit, just assign the return values of your functions to c1 ... and fixe your setbit and clear bit functions ... they don't return values, and if you were using proper warning settings in your compiler it would tell you that. I've made some stylistic changes:

/* your getbit works but is needlessly complex */
unsigned char getBit(unsigned char c, int n){
    return (c >> n) & 1; 

unsigned char clearBit(unsigned char c, int n){
    return c & ~(1 << n);
}

unsigned char setBit(unsigned char c, int n){
    return c | (1 << n);
} 

unsigned char xor4(unsigned char c1, unsigned char c2){
    for( int i = 0; i < 8; i += 2 )
        c1 = ( getBit(c1, i) ^ getBit(c2, i)
               ? setBit(c1, i);
               : clearBit(c1, i) );

    return c1;
}

Here's a faster solution:

unsigned char xor4(unsigned char c1, unsigned char c2){
    return (c1 & 0xAA) | ((c1 ^ c2) & 0x55);
}

Even faster:

unsigned char xor4(unsigned char c1, unsigned char c2){
    return (c2 & 0x55) ^ c1;
}

Upvotes: 1

Daniel Fischer
Daniel Fischer

Reputation: 183858

When you're calling setBit and clearBit, you are passing copies of c1 to these functions. Thus the value of c1 in xor4 is not changed at all.

Replace

 if((getBit(c1,i)^getBit(c2,i))) setBit(c1,i);
 else clearBit(c1,i);

with direct set or clear operations,

 if((getBit(c1,i)^getBit(c2,i))) c1 |= (1 << i);
 else c1 &= ~(1 << i);

(but, as was pointed out, just replacing the function with a simple c1 ^ c2 would be more efficient.)

Upvotes: 3

Related Questions