Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > C Programming > What's wrong with this code?

Reply
Thread Tools

What's wrong with this code?

 
 
xbyte
Guest
Posts: n/a
 
      09-29-2007
It's a problem from a book, and I can't figure it out.

4 words (4x8 bits) are packed into a 32-bit unsigned integer, and
you've gotta extract an indicated (by bytenum) word from it, AND the
following code is said to be written by a failed programmer:

int xbyte(unsigned word, int bytenum)
{
return (word >> (bytenum << 3)) & 0xFF;
}

IMHO, this is a perfect bulk of code, what's your opinion?

 
Reply With Quote
 
 
 
 
Ben Bacarisse
Guest
Posts: n/a
 
      09-29-2007
xbyte <(E-Mail Removed)> writes:

> It's a problem from a book, and I can't figure it out.
>
> 4 words (4x8 bits) are packed into a 32-bit unsigned integer, and
> you've gotta extract an indicated (by bytenum) word from it, AND the
> following code is said to be written by a failed programmer:
>
> int xbyte(unsigned word, int bytenum)
> {
> return (word >> (bytenum << 3)) & 0xFF;
> }
>
> IMHO, this is a perfect bulk of code, what's your opinion?


Perfect is a strong word! I'd say it captures the gist of the
problem but it is not portable and will exhibit undefined behaviour
is some cases.

Unsigned types are a natural choice when doing bit operations, so I'd
prefer an unsigned return type (unless the integer return is used to
signal an error using a negative value).

The bytenum can not be negative, so why not make that unsigned? In
any case, undefined behaviour awaits if bytenum is out of range.

Personally, I prefer '* 8' to '<< 3' when multiplying by 8.

C99 provides a good type to document the 32-bit assumption: uint32_t
but slightly greater portability can be got by using uint_least32_t.
Thus:

uint_least8_t xbyte(uint_least32_t word, unsigned bytenum)
{
assert(bytenum < 4);
return (word >> (bytenum * ) & 0xff;
}

or maybe:

int xbyte(uint_least32_t word, unsigned bytenum)
{
return bytenum < 4 ? (word >> (bytenum * ) & 0xff : -1;
}

In C90, I'd probably write:

unsigned char xbyte(unsigned long word, unsigned bytenum)
{
assert(bytenum < 4);
return (word >> (bytenum * ) & 0xff;
}

or

int xbyte(unsigned long word, unsigned bytenum)
{
return bytenum < 4 ? (word >> (bytenum * ) & 0xff : -1;
}

but I would never say any of these is perfect (for one thing that
would imply that code can be unambiguously ordered by "quality").

--
Ben.
 
Reply With Quote
 
 
 
 
Joachim Schmitz
Guest
Posts: n/a
 
      09-29-2007
"xbyte" <(E-Mail Removed)> schrieb im Newsbeitrag
news:(E-Mail Removed) ups.com...
> It's a problem from a book, and I can't figure it out.
>
> 4 words (4x8 bits) are packed into a 32-bit unsigned integer, and
> you've gotta extract an indicated (by bytenum) word from it, AND the
> following code is said to be written by a failed programmer:
>
> int xbyte(unsigned word, int bytenum)
> {
> return (word >> (bytenum << 3)) & 0xFF;
> }
>
> IMHO, this is a perfect bulk of code, what's your opinion?

The function should return a byte (unsigned char), but does return an int.
You'd need to check whether bytenum is in the range of 0-3
There's no quarantee that unsigned (int) is 32 bit long

Bye, Jojo


 
Reply With Quote
 
Malcolm McLean
Guest
Posts: n/a
 
      09-29-2007

"xbyte" <(E-Mail Removed)> wrote in message
news:(E-Mail Removed) ups.com...
> It's a problem from a book, and I can't figure it out.
>
> 4 words (4x8 bits) are packed into a 32-bit unsigned integer, and
> you've gotta extract an indicated (by bytenum) word from it, AND the
> following code is said to be written by a failed programmer:
>
> int xbyte(unsigned word, int bytenum)
> {
> return (word >> (bytenum << 3)) & 0xFF;
> }
>
> IMHO, this is a perfect bulk of code, what's your opinion?
>

The programmer might have failed the course, but the code is good enough to
be used even in a production environment. Because it is extremely unlikely
that it can fail usefully.

However bytenum should be checked to be within the range 0-3, word should
arguably be an unsigned long to guarantee at least 32 bits. I say "arguably"
because in fact an extremely low level function like this needs to be
efficient, and a conversion from int to long might be exactly what you don't
need if it is not a nop.
The identifier "word" is a bit unfortunate, since it looks like it might be
a user-defined basic type.

int is the conventional return type for functions returning chars, by the
way. You could argue this is a bad convention, but it allows -1 on error.

--
Free games and programming goodies.
http://www.personal.leeds.ac.uk/~bgy1mm

 
Reply With Quote
 
Flash Gordon
Guest
Posts: n/a
 
      09-29-2007
Malcolm McLean wrote, On 29/09/07 21:03:
>
> "xbyte" <(E-Mail Removed)> wrote in message
> news:(E-Mail Removed) ups.com...
>> It's a problem from a book, and I can't figure it out.
>>
>> 4 words (4x8 bits) are packed into a 32-bit unsigned integer, and
>> you've gotta extract an indicated (by bytenum) word from it, AND the
>> following code is said to be written by a failed programmer:
>>
>> int xbyte(unsigned word, int bytenum)
>> {
>> return (word >> (bytenum << 3)) & 0xFF;
>> }
>>
>> IMHO, this is a perfect bulk of code, what's your opinion?
>>

> The programmer might have failed the course, but the code is good enough
> to be used even in a production environment.


Depends on the environment. I can certainly see it being rejected at
review for using a left shift where what is really meant is multiplication.

> Because it is extremely
> unlikely that it can fail usefully.


As written it is unlikely to fail in any useful manner, certainly.

> However bytenum should be checked to be within the range 0-3, word
> should arguably be an unsigned long to guarantee at least 32 bits. I say
> "arguably" because in fact an extremely low level function like this
> needs to be efficient, and a conversion from int to long might be
> exactly what you don't need if it is not a nop.


Now explain how a conversion which does nothing can affect efficiency.
Especially as it would probably be called with an unsigned long as a
parameter in a project where they had made the parameter an unsigned long.

> The identifier "word" is a bit unfortunate, since it looks like it might
> be a user-defined basic type.


It does not look like that to anyone who reads it.

> int is the conventional return type for functions returning chars, by
> the way. You could argue this is a bad convention, but it allows -1 on
> error.


If there is no error checking then returning an unsigned long documents
what will be returned, if a check is added then returning an int and
using -ve values for error codes makes sense.
--
Flash Gordon
 
Reply With Quote
 
Richard
Guest
Posts: n/a
 
      09-29-2007
Flash Gordon <(E-Mail Removed)> writes:

> Malcolm McLean wrote, On 29/09/07 21:03:
>>
>> "xbyte" <(E-Mail Removed)> wrote in message
>> news:(E-Mail Removed) ups.com...
>>> It's a problem from a book, and I can't figure it out.
>>>
>>> 4 words (4x8 bits) are packed into a 32-bit unsigned integer, and
>>> you've gotta extract an indicated (by bytenum) word from it, AND the
>>> following code is said to be written by a failed programmer:
>>>
>>> int xbyte(unsigned word, int bytenum)
>>> {
>>> return (word >> (bytenum << 3)) & 0xFF;
>>> }
>>>
>>> IMHO, this is a perfect bulk of code, what's your opinion?
>>>

>> The programmer might have failed the course, but the code is good
>> enough to be used even in a production environment.

>
> Depends on the environment. I can certainly see it being rejected at
> review for using a left shift where what is really meant is
> multiplication.


Not if we are talking about shifting as opposed to multiplication. Using
the shift makes it obvious that we are shifting bits around. "*8" on the
other hand might be any arbitrary multiplication that just happens to be
a power of 2.
 
Reply With Quote
 
Willem
Guest
Posts: n/a
 
      09-29-2007
Richard wrote:
) Flash Gordon <(E-Mail Removed)> writes:
)> Malcolm McLean wrote, On 29/09/07 21:03:
)>>
)>> "xbyte" <(E-Mail Removed)> wrote in message
)>> news:(E-Mail Removed) ups.com...
)>>> It's a problem from a book, and I can't figure it out.
)>>>
)>>> 4 words (4x8 bits) are packed into a 32-bit unsigned integer, and
)>>> you've gotta extract an indicated (by bytenum) word from it, AND the
)>>> following code is said to be written by a failed programmer:
)>>>
)>>> int xbyte(unsigned word, int bytenum)
)>>> {
)>>> return (word >> (bytenum << 3)) & 0xFF;
)>>> }
)>>>
)>>> IMHO, this is a perfect bulk of code, what's your opinion?
)>>>
)>> The programmer might have failed the course, but the code is good
)>> enough to be used even in a production environment.
)>
)> Depends on the environment. I can certainly see it being rejected at
)> review for using a left shift where what is really meant is
)> multiplication.
)
) Not if we are talking about shifting as opposed to multiplication. Using
) the shift makes it obvious that we are shifting bits around. "*8" on the
) other hand might be any arbitrary multiplication that just happens to be
) a power of 2.

I direct your attention to what the code actually tries to do.
8 *is* an arbitrary multiplication that happens to be a power of two.
It could just as well have been 9.

To make the point more clear, a slightly more portable way to write
it would be:

return (word >> (bytenum * CHAR_BIT)) & UCHAR_MAX;

Although I'm sure there are several issues with that as well.


SaSW, Willem
--
Disclaimer: I am in no way responsible for any of the statements
made in the above text. For all I know I might be
drugged or something..
No I'm not paranoid. You all think I'm paranoid, don't you !
#EOT
 
Reply With Quote
 
=?iso-2022-kr?q?Harald_van_D=0E=29=26=0Fk?=
Guest
Posts: n/a
 
      09-29-2007
On Sun, 30 Sep 2007 01:32:04 +0200, Richard wrote:
> Flash Gordon <(E-Mail Removed)> writes:
>> Malcolm McLean wrote, On 29/09/07 21:03:
>>>
>>> "xbyte" <(E-Mail Removed)> wrote in message
>>> news:(E-Mail Removed) ups.com...
>>>> It's a problem from a book, and I can't figure it out.
>>>>
>>>> 4 words (4x8 bits) are packed into a 32-bit unsigned integer, and
>>>> you've gotta extract an indicated (by bytenum) word from it, AND the
>>>> following code is said to be written by a failed programmer:
>>>>
>>>> int xbyte(unsigned word, int bytenum) {
>>>> return (word >> (bytenum << 3)) & 0xFF;
>>>> }
>>>>
>>>> IMHO, this is a perfect bulk of code, what's your opinion?
>>>>
>>> The programmer might have failed the course, but the code is good
>>> enough to be used even in a production environment.

>>
>> Depends on the environment. I can certainly see it being rejected at
>> review for using a left shift where what is really meant is
>> multiplication.

>
> Not if we are talking about shifting as opposed to multiplication. Using
> the shift makes it obvious that we are shifting bits around. "*8" on the
> other hand might be any arbitrary multiplication that just happens to be
> a power of 2.


If you were writing the code for a 9-bit char 36-bit int machine, you
might end up with something like

int xbyte(unsigned word, int bytenum) {
return (word >> (bytenum * 9)) & 0x1FF;
}

"*8" is just any arbitrary multiplication that happens to be a power of
2, in this case.
 
Reply With Quote
 
Flash Gordon
Guest
Posts: n/a
 
      09-30-2007
Willem wrote, On 30/09/07 00:45:
> Richard wrote:
> ) Flash Gordon <(E-Mail Removed)> writes:
> )> Malcolm McLean wrote, On 29/09/07 21:03:
> )>>
> )>> "xbyte" <(E-Mail Removed)> wrote in message
> )>> news:(E-Mail Removed) ups.com...


<snip>

> )>>> return (word >> (bytenum << 3)) & 0xFF;


<snip>

> )>> The programmer might have failed the course, but the code is good
> )>> enough to be used even in a production environment.
> )>
> )> Depends on the environment. I can certainly see it being rejected at
> )> review for using a left shift where what is really meant is
> )> multiplication.
> )
> ) Not if we are talking about shifting as opposed to multiplication. Using
> ) the shift makes it obvious that we are shifting bits around. "*8" on the
> ) other hand might be any arbitrary multiplication that just happens to be
> ) a power of 2.
>
> I direct your attention to what the code actually tries to do.
> 8 *is* an arbitrary multiplication that happens to be a power of two.
> It could just as well have been 9.


Yes, that is indeed a correct expansion of my point, as was Harold's
response to Richard.

> To make the point more clear, a slightly more portable way to write
> it would be:
>
> return (word >> (bytenum * CHAR_BIT)) & UCHAR_MAX;
>
> Although I'm sure there are several issues with that as well.


It depends on the real requirement. If you are dealing with some format
where you have four 8 bit numbers packed in to an integer type then
using 8 and 0xFF is correct, if the requirement is 4 C bytes then your
code is correct.
--
Flash Gordon
 
Reply With Quote
 
Army1987
Guest
Posts: n/a
 
      09-30-2007
On Sat, 29 Sep 2007 08:43:44 -0700, xbyte wrote:

> It's a problem from a book, and I can't figure it out.
>
> 4 words (4x8 bits) are packed into a 32-bit unsigned integer, and
> you've gotta extract an indicated (by bytenum) word from it, AND the
> following code is said to be written by a failed programmer:
>
> int xbyte(unsigned word, int bytenum)
> {
> return (word >> (bytenum << 3)) & 0xFF;
> }
>
> IMHO, this is a perfect bulk of code, what's your opinion?

None. But if you replace `<< 3` with `* CHAR_BIT` and 0xFF with
UCHAR_MAX (these are in <limits.h>), you don't have to know how
big the word and the bytes are.
--
Army1987 (Replace "NOSPAM" with "email")
A hamburger is better than nothing.
Nothing is better than eternal happiness.
Therefore, a hamburger is better than eternal happiness.

 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
Have I bought wrong product? enquirer Wireless Networking 2 06-10-2005 10:59 PM
Zero Config keeps connecting to the wrong AP =?Utf-8?B?ZGdyaWZmaXRo?= Wireless Networking 2 03-04-2005 05:52 PM
Is XML Doc wrong or is Schema wrong? (or both) Matthew XML 7 01-07-2005 10:05 PM
wrong connection status Peter Welk Wireless Networking 0 12-22-2004 03:26 PM
XP SP2 Wrong IP on connection D Wells Wireless Networking 3 12-09-2004 03:35 AM



Advertisments