Closed Bug 208627 Opened 22 years ago Closed 22 years ago

Incorrect comparison in nsMailDatabase.cpp

Categories

(MailNews Core :: Database, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: tenthumbs, Assigned: Bienvenu)

Details

Attachments

(1 file)

Gcc says: nsMsgDBView.cpp:5204: warning: \ comparison of unsigned expression >= 0 is always true 541 *aResult = PR_ABS(actualFolderTimeStamp - folderDate) <= gTimeStampLeeway; which doesn't work because folderDate is unsigned. You probably want something like aResult = (folderDate - gTimeStampLeeway <= actualFolderTimeStamp) && (actualFolderTimeStamp <= folderDate + gTimeStampLeeway);
actually, it does work, at least on windows, because the code does the right thing. Both numbers are PRUint32's. Also, this code isn't in nsMsgDBView.cpp, it's in nsMsgDatabase.cpp I think perhaps the compiler is really complaining about this line: PRBool nsMsgDBView::IsValidIndex(nsMsgViewIndex index) { return ((index >=0) && (index < (nsMsgViewIndex) m_keys.GetSize())); which makes sense because index really is unsigned. Maybe there are two warnings and you've combined them somehow...
Yeah, nsMsgDBView.cpp is the warning just above the one I wanted. I can't copy and paste. :-) Sorry about that. The file in the summary is the correct one as is the extracted code. nsMailDatabase.cpp:541: warning: comparison of unsigned expression < 0 is always false Gcc is much stricter than most Windows compilers about unsigned comparisons and it prefers to evaluate expressiosn at compile time whenever it can. PR_ABS(a) is something like ((a < 0) ? -a : a) so the gcc warning means gcc knows a is always positive so it knows the -a is unreachable so it eliminates it and gcc compiles this as (a) which is not what you want. If actualFolderTimeStamp > folderDate then aResult = 0 always (well, for the most likely cases).
Let me make it clear, if either p or q are unsigned and p = q - 1, then gcc will evaluate p - q as UNINT_MAX not -1 so the code as it is doesn't work.
Attached patch proposed fixSplinter Review
this cast fixes the warning.
Comment on attachment 125140 [details] [diff] [review] proposed fix r/sr=sspitzer
Attachment #125140 - Flags: superreview+
Attachment #125140 - Flags: review+
fix checked in.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: