Closed Bug 757949 Opened 7 years ago Closed 7 years ago

GCC 4.6.3 build warning: nsXULDocument.cpp:2384:40: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]

Categories

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

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Noticed this GCC 4.6.3 build warning go by in my build-spew:
{
/mozilla/content/xul/document/src/nsXULDocument.cpp: In member function ‘nsresult nsXULDocument::PrepareToWalk()’:
/mozilla/content/xul/document/src/nsXULDocument.cpp:2384:40: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
}

...which points to this code:
> 2381     PRUint32 piInsertionPoint = 0;
> 2382     if (mState != eState_Master) {
> 2383         piInsertionPoint = IndexOf(GetRootElement());
> 2384         NS_ASSERTION(piInsertionPoint >= 0,
> 2385                      "No root content when preparing to walk overlay!");

This code has been like this since it was added back in 2006 in bug 319654
https://bugzilla.mozilla.org/attachment.cgi?id=245350&action=diff#mozilla/content/xul/document/src/nsXULDocument.cpp_sec10

Looks like we're trying to check whether nsDocument::IndexOf gave us a negative number (for failure), but we're failing to actually test that, because we're storing IndexOf's return value in an unsigned variable.
Attached patch fixSplinter Review
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #626544 - Flags: review?(bzbarsky)
Comment on attachment 626544 [details] [diff] [review]
fix

>     PRUint32 piInsertionPoint = 0;
>     if (mState != eState_Master) {
>-        piInsertionPoint = IndexOf(GetRootElement());
>-        NS_ASSERTION(piInsertionPoint >= 0,
>+        PRInt32 indexOfRoot = IndexOf(GetRootElement());
>+        NS_ASSERTION(indexOfRoot >= 0,
>                      "No root content when preparing to walk overlay!");
>+        piInsertionPoint = indexOfRoot;

Peanut gallery observations:
Won't that just replace one warning with another (assigning unsigned to signed)?

Why do you need indexOfRoot anyway?  Why not have:

NS_ASSERTION(IndexOf(GetRootElement()) >= 0, ...); 
piInsertionPoint = IndexOf(GetRootElement());

This is debug code anyway.
(In reply to Alex Vincent [:WeirdAl] from comment #2)
> Won't that just replace one warning with another (assigning unsigned to
> signed)?

Not in GCC, at least. It trusts you to on unsigned-to-signed conversions.

(MSVC may warn about that, though -- I'm not sure -- but if it does, we're already chin-deep in those warnings, I'm afraid. :-/

I try to avoid casts when they're not needed, so as not to introduce maintenance overhead if the type of a variable were to change (e.g. if piInsertionPoint were to change to a PRUint64 at some point in the future, for some reason)

> Why do you need indexOfRoot anyway?  Why not have:
> 
> NS_ASSERTION(IndexOf(GetRootElement()) >= 0, ...); 
> piInsertionPoint = IndexOf(GetRootElement());

That'd be be duplicating (a small amount of) code. If we hypothetically changed that "IndexOf(GetRootElement())" call down the line, we'd need to remember to make the change in two places.  This way, that's not a concern.

And any sane compiler will optimize away 'indexOfRoot' to be equivalent to the already-existing code anyway, in a non-debug build.

> This is debug code anyway.

Sure. So, probably best not to spend too much time over-thinking the fix. :)
(In reply to Daniel Holbert [:dholbert] from comment #4)
> (In reply to Alex Vincent [:WeirdAl] from comment #2)
> > Won't that just replace one warning with another (assigning unsigned to
> > signed)?
> 
> Not in GCC, at least. It trusts you to on unsigned-to-signed conversions.

(er, I meant "signed-to-unsigned", regarding this case at least)
https://hg.mozilla.org/mozilla-central/rev/86edd7136c3b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.