I ran across some code today that puzzled me. It was an infinite loop that used a counter to determine if things took too long. Something like this:
int main()
{
int count;
int status;
count = 0;
do
{
status = GetStatus();
count++;
} while( status == 0 );
if (count < 0)
{
printf("Time out! count = %d\n", count);
return EXIT_FAILURE;
}
printf("Done. count = %d\n", count);
return EXIT_SUCCESS;
}
The code would read some hardware status (“GetStatus() in this example) and drop out of the do/while loop once it had a value. Inside that loop, it increments a count variable. After done, it would check for “count < 0” and exit with a timeout if that were true.
Count is only incremented, so the only way count could ever be less than zero is if it incremented so high it rolled over. With a signed 8-bit value (int8_t), you count from 0 to 127, then it rolls over to -128 and counts back up to 0.
So with an “int” being a 32-bit value (on the system I am using), the rollover happens at 2147483647.
And that seems like quite a weird value.
But I suppose it it took that long, it certainly timed out.
I think if was going to do it, I would have probably used an unsigned integer, and just specified an appropriate value:
unsigned int count; ... if (count > 10000) { printf("Time out! count = %u\n", count); return EXIT_FAILURE; }
What says you?
If it can actually go 2^31 times around before status changes, why not 2^32 times, in which case the timeout check would fail? (Or would it? Signed integer overflow is officially undefined behavior, giving the compiler the right to generate code to order pizza with anchovies or launch the nuclear warheads.) Better to do something like
count = min(MAX_COUNT, count + 1);
and then check for count == MAX_COUNT.
The whole structure looks weired to me. The loop for checking the status will not be left if the status remains 0. Count may wrap multiple times and finally has a meaningless value.
As previously mentioned a minimum function should help to get rid of it. Anyway, why not to leave the status loop if the timeout is reached? Something like
while (status == 0 && count < 10000)
if (status == 0) /* timeout */ ….
I wouldn't trust the hardware to ever change the status in terms of reliable software.
And that’s actually a related problem. This is based on Ethernet code and it would get stuck in that loop forever if it got PoE but no IP address. And it’s a commercial IP stack.
Yes that code is definitely crap, for reasons mentioned already.
Sometimes you just have to hope there’s something really clever being done that you can’t see at first.