221289 - Test for unsigned >= 0 in content/base/src/nsGenericElement.cpp

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[tenthumbs]

Gcc says:

nsGenericElement.cpp:294: warning: \
 comparison of unsigned expression >= 0 is always true

Code is:

294   for (PRUint32 i = childCount; i-- != 0; ) {
295     aContent->RemoveChildAt(i, PR_TRUE);

The loop never terminates. Surprising this hasn't bitten someone. Patch upcoming.

Comment 1

[tenthumbs]

Attachment: simple patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132682 [details] [diff] [review]
simple patch

r+sr=bzbarsky.	It's not bitten anyone because it's a recent change, and
because DOM3 is rarely used... ;)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Checeked in.  Thanks for the patch!

In general, though, please ask for reviews to get people to notice the patch...

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

Note that this was caused by jst's patch for bug 215981.


Also note that this patch causes the loop to run one too many times each pass. 
Please be more careful when fixing simple things like this.


>   PRUint32 childCount = aContent->GetChildCount();
> 
>-  for (PRUint32 i = childCount - 1; i >= 0; --i) {
>+  for (PRUint32 i = childCount; i-- != 0; ) {
>     aContent->RemoveChildAt(i, PR_TRUE);
>   }


We really don't want to remove the child at the |aContent->GetChildCount()|th
index since there isn't one.  The loop should be


  while (childCount-- > 0) {
    aContent->RemoveChildAt(childCount, PR_TRUE);
  }

Comment 5

[Boris Zbarsky [:bzbarsky]]

caillon, 

>+  for (PRUint32 i = childCount; i-- != 0; ) {

and

  while (childCount-- > 0) {

are actually completely equivalent...

Comment 6

[Hixie (not reading bugmail)]

bz: except the second one is about 10 times easier to read. :-P

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

the difference is that the second one will modify childCount, so they are not
_completely_ equivalent

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Hm, misread that.  Yes, of course they are equal.  I'll just make the change
though to make it 10 times easier to read as Hixie notes, and get rid of the
extra variable.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Yeah, since we never use childCount after that the switch can be made (and is
more readable)...

I was just defending myself and tenthumbs against charges of illogic.  ;)

Comment 10

[tenthumbs]

> Also note that this patch causes the loop to run one too many times 
> each pass.
> Please be more careful when fixing simple things like this.

If you had asked I would have told you that I tested for this and the 
loop enumerated the same values either way. I may be sloppy but not that 
sloppy.