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

VERIFIED FIXED in mozilla1.3beta

Status

()

Core
CSS Parsing and Computation
P1
minor
VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: tenthumbs, Assigned: bz)

Tracking

Trunk
mozilla1.3beta
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

16 years ago
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 ...
->bz
Assignee: dbaron → bzbarsky
lxr finds:
/content/html/style/src/nsCSSLoader.cpp, line 254 -- PRUint32 mPendingChildren;

so yeah, this is currently unsigned.
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.
Priority: -- → P1
Target Milestone: --- → mozilla1.3beta
(Reporter)

Comment 4

16 years ago
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.
A negative amount of pending children does not make any sense, why would you
make it signed?
(Reporter)

Comment 6

16 years ago
Precisely because negative children are invalid and because it's easy 
to test for negative values.
It's unsigned because it's conceptually unsigned.  That's just the data type 
that best fits the variable's intended use.
(Reporter)

Comment 8

16 years ago
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.
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_.
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.
(Reporter)

Comment 11

16 years ago
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

16 years ago
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.
Attachment #110679 - Flags: superreview?(peterv)
Attachment #110679 - Flags: review?(bugmail)
Summary: Useless assertion in content/html/style/src/nsCSSLoader.cpp → [FIX]Useless assertion in content/html/style/src/nsCSSLoader.cpp
(Reporter)

Comment 14

16 years ago
> 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.
hmm.. good point.  Consider the change to !=0 made.
Attachment #110679 - Flags: superreview?(peterv) → superreview+
Summary: [FIX]Useless assertion in content/html/style/src/nsCSSLoader.cpp → [FIXr]Useless assertion in content/html/style/src/nsCSSLoader.cpp
checked in with the change to != 0.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 17

16 years ago
verifing as per developers comments 
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.