Curiousity in packet.cpp

[Jaerin]

I was taking a look at the code because when I compile it on FreeBSD 4.7 it compiled fine, but there were a couple of warnings.



packet.cpp: In member function `void EQPacket::dispatchZoneData(unsigned int, uint8_t*, unsigned char)':
packet.cpp:1805: warning: comparison is always true due to limited range of data type
packet.cpp:1805: warning: comparison is always true due to limited range of data type


So I took a look in packet.cpp and here's what I saw:



void EQPacket::dispatchZoneData (uint32_t len, uint8_t *data, uint8_t dir)
...snip...
struct timeOfDayStruct eqTime;
getEQTimeOfDay( timeCurrent, &eqTime);

if (eqTime.hour >= 0 && eqTime.minute >= 0)
{
sprintf(timeMessage,"EQTime [%02d:%02d %s]",
(eqTime.hour % 12) == 0 ? 12 : (eqTime.hour % 12),
(eqTime.minute),
((eqTime.hour >= 12 && eqTime.hour << 24) ||
(eqTime.hour == 24 && eqTime.minute == 0)) ? "pm" : "am");
emit eqTimeChangedStr(QString (timeMessage));
}
...snip...


According to the struct definition eqTime.hour and eqTime.minute are both typed as uint8_t. Which if you dig into the sys/inttypes.h, sys/types.h, machine/types.h, and machine/ansi.h files on my FreeBSD box you find they are unsigned chars also known as unsigned ints. In the highlighted test above if both are unsigned ints you would never have a negative value. So I'm curious as to what the test is trying to acheive.

I was thinking that perhaps it's a difference between the Linux system and FreeBSD. If uint8_t was defined as a signed int then it would make sense.


Thanks

Jaerin

[fester]

You should use the search button

http://seq.sourceforge.net/showthread.php?s=&threadid=2806&highlight=eqTime.hour

[Jaerin]

Okay so you subtract 1 hour from eqTime.hour to get the proper time. That still doesn't explain how an unsigned int is ever going to fail the if (eqTime.hour >= 0 && eqTime.minute >= 0) test. eqTime.hour and eqTime.minute can never be negative hence never fail the test. If eqTime.hour or eqTime.minute should be allowed to be negative values then in everquest.h the timeOfDayStruct should be as follows:



struct timeOfDayStruct
{
/*0000*/ uint8_t opCode; // 0xf2
/*0001*/ uint8_t version; // 0x20
/*0002*/ int8_t hour; // Hour (1-24)
/*0003*/ int8_t minute; // Minute (0-59)
/*0004*/ uint8_t day; // Day (1-28)
/*0005*/ uint8_t month; // Month (1-12)
/*0006*/ uint16_t year; // Year
/*0008*/ uint16_t unknown0016; // Placeholder
};


Jaerin

[JoeBloe]

These structs are not static.

Better to leave the test in in case it ever changes.

And this certainly is not worth worrying about the few cycle to do a compare.

[Jaerin]

Your not getting the point.

As it currently is the test will NEVER fail. It's just not possible. So the test is pointless.

All I'm saying is if this test were to be in a position to fail the app would have core dumped already.

So all I'm saying is we should either A) Change the struct or B) Fix/Remove the test.

Rule #1 in programming, "Do it right the first time"

If you kludge up your code from the beginning it's only going to get worse as time goes on.

Jaerin

[high_jeeves]

Jaerin:

Lemme guess, you are a 21 year old TA of a CS 101 class? Here is the deal... that used to be a signed value, therefore, the test was necessary... It was changed a while ago to be an unsigned value, but not all of the sanity checks were removed. Its not a kludge. It only generates a warning (and gets your panties in a bunch). it doesnt hurt anything at all. if that warning bothers you so much, feel free to send in a patch...

I mean, really.. if you code in the real world, and this is something that upsets you, I recommend that you take a long vacation..

--Jeeves

[Jaerin]

I'm not 21...and no I'm not a TA of a CS101 class. I do believe in good coding practices. I have read and modified my fair share of really bad code, and I wouldn't say this is anywhere close to bad code.

I do code in the real world and I agree that it doesn't hurt anything, but I was simply pointing out the unnecessary test. It doesn't really bother me that much, but I figured if someone mentioned it that either A) a reason would be given that it's there or B) The fact that it was uneccessary would be realized and it would be included in the next code change.

high_jeeves you accomplished A. There was a point to it being there at one point but not any longer. You confirmed exactly what I was saying.

If you also code in the real world you will know that being wasteful of CPU cycles can lead to lots of problems down the road. Will this make a dramatic difference? No probably not, but everything that can be done, should be done unless it directly affects the overall goal. For example if I mentioned that we needed to dramatically change one of the fundamental prcedures such that it would require a lot of time and effort just to save a few CPU cycles. Then no, but this is a small easy fix that takes no time.

[PopNFresh]

Originally posted by high_jeeves
Jaerin:

Lemme guess, you are a 21 year old TA of a CS 101 class? Here is the deal... that used to be a signed value, therefore, the test was necessary... It was changed a while ago to be an unsigned value, but not all of the sanity checks were removed. Its not a kludge. It only generates a warning (and gets your panties in a bunch). it doesnt hurt anything at all. if that warning bothers you so much, feel free to send in a patch...

I mean, really.. if you code in the real world, and this is something that upsets you, I recommend that you take a long vacation..

--Jeeves


OMG, I agree with Jeeves for once and even find his post enlightening and worthwhile.

I MUST BE DRUNK or its time to get drunk one or the other. ;-)

[casey]

on my FreeBSD box you find they are unsigned chars also known as unsigned ints.

if you look really hard, you'll find thats not really the case.

chars are 8 bits wide, ints are not, not even the short ones.

[throx]

Testing unsigned values are non-negative makes Baby Jesus cry.

[Jaerin]

Well of course the bit width wasn't in question. It was the signed/unsigned bit.

Jaerin

[Jillian]

Originally posted by Jaerin
If you also code in the real world you will know that being wasteful of CPU cycles can lead to lots of problems down the road. Will this make a dramatic difference? No probably not...
I'm glad this thread really isn't about wasteful cpu cycles... Acutally I also try to get rid of all my warnings when I've got the time.

[Raistlin]

Originally posted by throx
Testing unsigned values are non-negative makes Baby Jesus cry.

And trolling the code for useless checks and pointing out the wastefullness of a single if statement in a huge program makes Baby Jesus cry lots!

Besides, if GCC can't figure out that that code is useless and remove it under ANY, and I do mean ANY compiler optimization cycle I HIGHLY suggest we find another compiler.

Let me explain what the code:

if (<unsigned int> >= 0)
{
<statements>
}

turns out to be in the compiled code.

<statements>

In the words of one board poster.

"Move along, nothing to see here."

[casey]

Originally posted by Raistlin


And trolling the code for useless checks and pointing out the wastefullness of a single if statement in a huge program makes Baby Jesus cry lots!

Besides, if GCC can't figure out that that code is useless and remove it under ANY, and I do mean ANY compiler optimization cycle I HIGHLY suggest we find another compiler.

Let me explain what the code:

if (<unsigned int> >= 0)
{
<statements>
}

turns out to be in the compiled code.

<statements>

In the words of one board poster.

"Move along, nothing to see here."

w
o
w

....

did you try your best to post without actually looking at the thread?

he didnt troll the code looking for the error, GCC *warned* him that the check was pointless and it then proceeded to optimize it out.

read before posting the next time, and you wont look like such a moron.

[Raistlin]

Sorry Casey, I obviously didn't make the point I wanted to make, let me try one more time.

He was posting that the check was wasting CPU cycles in several of his posts. I was simply pointing out that it's impossible for this check to have wasted cpu cycles as it would simply be optimized out under any average modern day compiler and therefore doesn't hurt ANYTHING Being left in, other than making the code more readable...and with his "insistance" that this should be removed (As if this were the top priority of the Dev team at the moment) I felt the sarcasm warented.

And to readable, when I read that line I know that this number must be greater than or equal to 0...you could argue that by reading Unsigned in the structure def you could infer this, and i'd agree, but in my experience self documenting code is worth it's weight in gold for people trying to help in development....expecially if it doesn't waste anything...and this doesn't waste a thing.

I'll take this as a lesson in "sarcasm isn't always the best way to get your point across."

Thanks for the Correction Casey.

[Jaerin]

Originally posted by Raistlin
...He was posting that the check was wasting CPU cycles in several of his posts. I was simply pointing out that it's impossible for this check to have wasted cpu cycles as it would simply be optimized out under any average modern day compiler and therefore doesn't hurt ANYTHING Being left in, other than making the code more readable...and with his "insistance" that this should be removed (As if this were the top priority of the Dev team at the moment) I felt the sarcasm warented.


In my defense I in no way insinuated that anything should have been done about this in my initial post. It was actually an inquiry of what was intended by the code. Once that was answered I posted a patch for it. I wasn't requesting the devs do anything about it at all. I know that it wasn't even the bottom priority on the dev list. Since I don't have even a basic understanding of what most of the SEQ code does I can't help the devs with the actual fixing of the code, but that doesn't mean I'm not going to do as much as I *CAN* do to make the code better.

Which is much more than most of the trolls around here.
**NOTE This comment is not directed at anyone in particular***

Jaerin

[Pigeon]

It *does* waste cpu cycles.

It wastes .000002 seconds worth every time you compile showeq.

To say nothing of all the cycles, and bandwidth, it's wasted on all our computers displaying and updating this thread, not to mention human brain-cycles.

:D

[Jillian]

Originally posted by Pigeon
To say nothing of all the cycles, and bandwidth, it's wasted on all our computers displaying and updating this thread, not to mention human brain-cycles. Ok, that's disturbing.

[raytrace]

This little issue is known. Look under patches link.

Hmm.... well I was going to cut and paste the HTML from the patch page but I see the HTML is turn off for the forum.

Oh well, here it is without HTML.

Date: 2003-02-28 15:34
Priority: 5
Submitted By: Charles Vraspir (jaerin)
Assigned To: Nobody/Anonymous (nobody)
Category: bugfix
Status: Open

Summary:
Remove sanity check from packet.cpp
packet.cpp contains a sanity check to see if
eqTime.hour and eqTime.minute are negative. Both of
these values are typed as uint8_t (unsigned char) and
therefore can never be negative. It's my understanding
this is a leftover from when they were signed chars.
This check causes a few warnings when compiling
because the test can never fail. Not truely a bug
persay, but a little housecleaning.

This patch is a diff that takes out this check.

[wizard]

That patch was submitted by the exact person who started this thread...

[raytrace]

DOH!!!

2+2=5 right?

LOL

[wizard]

In some alternate universes i wouldn't be surprised....