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 ...
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
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?
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.
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.
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.
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.
16 years ago
16 years ago
Summary: Useless assertion in content/html/style/src/nsCSSLoader.cpp → [FIX]Useless assertion in content/html/style/src/nsCSSLoader.cpp
Attachment #110679 - Flags: review?(bugmail) → review+
> 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+
16 years ago
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
verifing as per developers comments
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.