c - Shifting a char value by 24 bits -
static unsigned int read24(unsigned char *ptr) { unsigned char b0; unsigned char b1; unsigned int b2; b0 = *ptr++; //b0 = 00 b1 = *ptr++; //b1 = aa b2 = *ptr; //b2 = bb b0 = (b0 & 0x000000ff) ; b1 = (b1 << 8) & 0x0000ff00); b2 = (b2 & 0x000000ff) << 16; return (b0 | b1 | b2) ; } code 2
static unsigned int read24(unsigned char *ptr) { unsigned char b0; unsigned char b1; unsigned char b2; b0 = *ptr++; b1 = *ptr++; b2 = *ptr; return ( ((b0 & 0x000000ff)) | ((b1 & 0x000000ff) << 8 | ((b2 << 16) & 0x00ff0000) | ); } here confused shifting operation done b1 , b2. shifting char b1 8 or more 8 make value zero? better strategy,first shifting , anding or first anding , shifting. want return value should bbaa00. have done right thing?
in case wrong ..is second code correct?
the asymmetry in handling of shift-by-8 , shift-by-16 odd, harmless long sizeof(int) >= 2 (assuming char_bit == 8). masking little odd; intention ensure that shift done on larger type, guaranteed since operands converted int. however, assignments b0, b1, , b2 undo done; b0 unchanged, , other 2 zeroed.
the code should more this:
static unsigned int read24(unsigned char *ptr) { unsigned char b0; unsigned char b1; unsigned int b2; // assert(sizeof(b2) >= 4); b0 = *ptr++; //b0 = 00 b1 = *ptr++; //b1 = aa b2 = *ptr; //b2 = bb b2 <<= 16; b2 |= (b1 << 8) & 0xff00; // mask necessary if sizeof(int) == 2 b2 |= b0; return b2; } it written more compactly as:
static unsigned int read24(unsigned char *ptr) { unsigned char b0 = *ptr++; unsigned char b1 = *ptr++; unsigned int b2 = *ptr; // assert(sizeof(b2) >= 4); return (b2 << 16) | (b1 << 8) | b0; } code 2
your second code fragment correct, return expression wrong. is:
return ( ((b0 & 0x000000ff)) | ((b1 & 0x000000ff) << 8 | // missing ) ((b2 << 16) & 0x00ff0000) | // extraneous | ); it should be:
static unsigned int read24(unsigned char *ptr) { unsigned char b0 = *ptr++; unsigned char b1 = *ptr++; unsigned char b2 = *ptr; return ( ((b0 & 0x00ff)) | ((b1 & 0x00ff) << 8) | ((b2 << 16) & 0x00ff0000)); } there missing parenthesis , extraneous | in return expression. if going down route, i'd use:
static unsigned int read24(unsigned char *ptr) { unsigned char b0 = *ptr++; unsigned char b1 = *ptr++; unsigned char b2 = *ptr; assert(sizeof(int) >= 4); return ( ((b0 << 0) & 0x0000ff) | ((b1 << 8) & 0x00ff00) | // 0xff00 instead of 0x00ff! ((b2 << 16) & 0xff0000)); } this preserves symmetries. << 0 optimized no-op, of course, without optimization turned on.
is b1 << 8 undefined behaviour?
since people seem under illusion b1 << 8 undefined behaviour when b1 unsigned char, let me quote standard:
iso/iec 9899:2011 §6.5.7 shift operators
¶3 integer promotions performed on each of operands. type of result of promoted left operand. if value of right operand negative or greater or equal width of promoted left operand, behavior undefined.
¶4 result of
e1 << e2e1left-shiftede2bit positions; vacated bits filled zeros. ife1has unsigned type, value of resulte1 × 2e2, reduced modulo 1 more maximum value representable in result type. ife1has signed type , non-negative value, ,e1 × 2e2representable in result type, resulting value; otherwise, behavior undefined.
so, unsigned char promoted int before shift occurs (hence comment sizeof(int) >= 2). if sizeof(int) == 2, left-shifting value in b1 in range 0x80..0xff leads undefined behaviour (ub). you'd have decide whether risk acceptable, or you'd cast b1 unsigned before shifting:
(unsigned)b1 << 8 in original code:
b1 = (b1 << 8) & 0x0000ff00); (which not compile: should b1 = (b1 << 8) & 0x0000ff00;!), assignment damage (leaving result zero), not shift.