Last Comment Bug 757949 - GCC 4.6.3 build warning: nsXULDocument.cpp:2384:40: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
: GCC 4.6.3 build warning: nsXULDocument.cpp:2384:40: warning: comparison of un...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86_64 Linux
-- normal (vote)
: mozilla15
Assigned To: Daniel Holbert [:dholbert] (vacation, returning 2/27)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: buildwarning 319654
  Show dependency treegraph
 
Reported: 2012-05-23 12:08 PDT by Daniel Holbert [:dholbert] (vacation, returning 2/27)
Modified: 2012-05-25 08:35 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (1.21 KB, patch)
2012-05-23 12:12 PDT, Daniel Holbert [:dholbert] (vacation, returning 2/27)
bzbarsky: review+
Details | Diff | Splinter Review

Description User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2012-05-23 12:08:13 PDT
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.
Comment 1 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2012-05-23 12:12:37 PDT
Created attachment 626544 [details] [diff] [review]
fix
Comment 2 User image Alex Vincent [:WeirdAl] 2012-05-23 12:20:43 PDT
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 3 User image Boris Zbarsky [:bz] (still a bit busy) 2012-05-23 12:20:50 PDT
Comment on attachment 626544 [details] [diff] [review]
fix

r=me
Comment 4 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2012-05-23 12:29:51 PDT
(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. :)
Comment 5 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2012-05-23 12:30:30 PDT
(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)
Comment 6 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2012-05-24 10:51:59 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/86edd7136c3b
Comment 7 User image Ed Morley [:emorley] 2012-05-25 08:35:39 PDT
https://hg.mozilla.org/mozilla-central/rev/86edd7136c3b

Note You need to log in before you can comment on or make changes to this bug.