187002 - [FIXr]Useless assertion in content/html/style/src/nsCSSLoader.cpp

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[tenthumbs]

I'm putting this in StyleSystem because it comes from the fix for bug 107567.

  nsCSSLoader.cpp:1715: warning: comparison of unsigned expression >= 0 is
always true


  NS_ASSERTION(aLoadData->mPendingChildren >= 0,
               "Negatively many kids?");

Gcc 3.2.1 thinks mPendingChildren is unsigned so this is useless as it stands.
Of course, if mPendingChildren is supposed to be signed ...

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

->bz

Comment 2

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

lxr finds:
/content/html/style/src/nsCSSLoader.cpp, line 254 -- PRUint32 mPendingChildren;

so yeah, this is currently unsigned.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Ah, yes.  I should be asserting at the site where I decrement that it's not 
zero before decrementing, instead of what I do now.

Comment 4

[tenthumbs]

I do have to ask why mPendingChildren is unsigned. You'll run out of 
memory long before you have 2^31 children so allowing 2^32 is rather 
silly.

Comment 5

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

A negative amount of pending children does not make any sense, why would you
make it signed?

Comment 6

[tenthumbs]

Precisely because negative children are invalid and because it's easy 
to test for negative values.

Comment 7

[Boris Zbarsky [:bzbarsky]]

It's unsigned because it's conceptually unsigned.  That's just the data type 
that best fits the variable's intended use.

Comment 8

[tenthumbs]

I suggest that humans, even programmers, do _not_ think in unsigned 
terms and using unsigned quantities when they are not required just 
leads to errors, bugs, and subtle design flaws.

Comment 9

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

I suggest that humans use the appropriate type, which is unsigned in this case.
Using a signed type just makes people wonder what a negative children count
means, because that concept makes _no sense_.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Um... since I get to maintain this code and I _do_ think in unsigned terms more 
easily, I get to keep it as I like it.  If you want to maintain this code 
instead of me, let me know and then you get to choose the data types.

Comment 11

[tenthumbs]

I didn't mean to offend you. If you wrote it then, sure, do what you 
want. This bug is just an example of what can go wrong.

Comment 12

[Jesse Ruderman]

As far as I can tell, this is an example of a compiler finding a bug that
wouldn't have been found if the type had been signed.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: more like that....

Comment 14

[tenthumbs]

> As far as I can tell, this is an example of a compiler finding a bug 
> that wouldn't have been found if the type had been signed.

Actually, this just shows that even the designer can forget what he
originally conceived. Humans are fallible and tend to fallback on what
they learned first. BTW, I don't think you get those warnings with the 
usual options.

Personally, I would prefer it said
   data->mParentData->mPendingChildren != 0
rather that > 0 because ">" can imply that "<" is also meaningful. You 
don't want someone in the future making the same mistake.

Comment 15

[Boris Zbarsky [:bzbarsky]]

hmm.. good point.  Consider the change to !=0 made.

Comment 16

[Boris Zbarsky [:bzbarsky]]

checked in with the change to != 0.

Comment 17

[Madhur Bhatia]

verifing as per developers comments