The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla15

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 1 bug)

Trunk
mozilla15
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 187528
(Assignee)

Comment 1

5 years ago
Created attachment 626544 [details] [diff] [review]
fix
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.
Comment on attachment 626544 [details] [diff] [review]
fix

r=me
Attachment #626544 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 4

5 years ago
(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. :)
(Assignee)

Comment 5

5 years ago
(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)
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/86edd7136c3b
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/86edd7136c3b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.