221292 - Test for unsigned >=0 in mozilla/content/events/src/nsEventStateManager.cpp

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[tenthumbs]

Gcc says:

nsEventStateManager.cpp:3581: warning: \
 comparison of unsigned expression >= 0 is always true

Code is:

3579  PRInt32 increment = forward ? 1 : - 1;
3580  PRInt32 start = index < count ? index + increment : (forward ? 0 : count - 1);
3581  for (index = start; index < count && index >= 0; index += increment) {
3582  //Iterate over the children.

I don't understand what's supposed to happen but (index >= 0) == 1 seems like a
bad thing.

Comment 1

[Nicholas Allen]

I think the index >= 0 can just be struck.  It looks like it's supposed to
terminate the loop in the backwards case.  But since index will be MAXINT when
the loop should terminate, that means index >= count and the first test will
catch it.  Should probably have |PRUint32 start| though.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yeah, it does look like the >= 0 check can just be removed, and a comment to
that effect added. Jst?  Thoughts?

Comment 3

[Mats Palmgren (inactive)]

I agree with Nick, the "index >= 0" test can be removed.
"start" can be eliminated too:

3580  index = index < count ? index + increment : (forward ? 0 : count - 1);
3581  for (; index < count; index += increment) {

Please also remove the space between the minus and 1 on line 3579.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Could someone attach a patch, then?  I can hardly review my own patch... ;)

Comment 5

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Comment 6

[Mats Palmgren (inactive)]

Tested by tabbing back and forth over an image map with zero or more areas.
Just to be sure ;)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132698 [details] [diff] [review]
Patch rev. 1

r+sr=bzbarsky

Comment 8

[Boris Zbarsky [:bzbarsky]]

Changed "follwing" to "following" and checked in.