287540 - Fix various _C++_ compiler warnings from my Windows non-debug build

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Serge Gautherie (:sgautherie)]

Comment 1

[Serge Gautherie (:sgautherie)]

<http://tinderbox.mozilla.org/SeaMonkey/warn1111652820.24346.html>
{{
scott

3.	extensions/spellcheck/src/mozInlineSpellChecker.cpp:718 (See build log excerpt)
	Comparison between signed and unsigned integer expressions
	

716 textChars = text.get();
717 textLength = text.Length();
718 if (aOffset == -1 || aOffset >= textLength) 
719   aOffset = textLength - 1;
}}

Scott,
I guess I figured out the 3 other warnings in this file, but
I'm not sure on how to fix this one properly: helpwanted.

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <mozInlineSpellChecker.cpp> (3/4) [Checkin: See comment 33]

"Self-Compiled [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b)
Gecko/20050324] (<-- v1.8b1 !) (W2Ksp4)"

Fixes 3 out of 4 warnings. (4th: see comment 1)

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) <nsXPInstallManager.cpp> [Checked in: Comment 30]

"Self-Compiled [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b)
Gecko/20050324] (<-- v1.8b1 !) (W2Ksp4)"

Fixes
{{
d:/mozbuild\mozilla\xpinstall\src\nsXPInstallManager.cpp(163) : warning C4018:
'<' : signed/unsigned mismatch
}}

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) <nsAppRunner.cpp>

"Self-Compiled [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b)
Gecko/20050324] (<-- v1.8b1 !) (W2Ksp4)"

Fixes
{{
d:/mozbuild\mozilla\xpfe\bootstrap\nsAppRunner.cpp(1618) : warning C4101: 'i' :
unreferenced local variable
}}

Comment 5

[[e-mail]]

Comment on attachment 179177 [details] [diff] [review]
(Cv1) <nsAppRunner.cpp>

Both MOZ_WIDGET_GTK and MOZ_WIDGET_GTK2 define MOZ_X11 so imho the real bug is
that the line was put in the wrong place.

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to comment #5)
> (From update of attachment 179177 [details] [diff] [review] [edit])
> Both MOZ_WIDGET_GTK and MOZ_WIDGET_GTK2 define MOZ_X11 so imho the real bug is
> that the line was put in the wrong place.

Right, sorry I didn't take the time to check bug 258055 before: revised patch
expected tomorrow.

(configure file has two more X11 targets...)
{{
11876 if test "$MOZ_ENABLE_GTK" \
11877 || test "$MOZ_ENABLE_QT" \
11878 || test "$MOZ_ENABLE_XLIB" \
11879 || test "$MOZ_ENABLE_GTK2"
11880 then
11881     cat >> confdefs.h <<\EOF
11882 #define MOZ_X11 1
11883 EOF
11884 
11885     MOZ_X11=1
11886 fi
}}

Comment 7

[Serge Gautherie (:sgautherie)]

Attachment: (Cv2) <nsAppRunner.cpp>

"Self-Compiled [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b)
Gecko/20050324] (<-- v1.8b1 !) (W2Ksp4)"

Av1, with comment 5 suggestion.

Comment 8

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) <intl/*> (1/7)

"Self-Compiled [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b)
Gecko/20050324] (<-- v1.8b1 !) (W2Ksp4)"

Fixes
{{
d:/mozbuild\mozilla\intl\locale\src\nsLanguageAtomService.cpp(129) : warning
C4101: 'res' : unreferenced local variable
}}

Comment 9

[timeless]

Comment on attachment 179272 [details] [diff] [review]
(Cv2) <nsAppRunner.cpp>

Qt at the very least can happen in cases that are not x11.

theoretically gtk(2) can too, although i'm not sure how color maps fair wrt non
x11.

Comment 10

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1a) <intl/*> (4/7) [Checked in: Comment 25]

"Self-Compiled [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b)
Gecko/20050324] (<-- v1.8b1 !) (W2Ksp4)"

Fixes
{{
d:/mozbuild\mozilla\intl\uconv\ucvlatin\nsUnicodeToTSCII.cpp(216) : warning
C4309: '=' : truncation of constant value
d:/mozbuild\mozilla\intl\uconv\ucvlatin\nsUnicodeToTSCII.cpp(249) : warning
C4309: '=' : truncation of constant value
d:/mozbuild\mozilla\intl\uconv\ucvlatin\nsUnicodeToTSCII.cpp(256) : warning
C4309: '=' : truncation of constant value

d:/mozbuild\mozilla\intl\locale\src\nsLanguageAtomService.cpp(129) : warning
C4101: 'res' : unreferenced local variable
}}
based on
<http://lxr.mozilla.org/mozilla/source/intl/uconv/ucvlatin/tamil.h#56>.

helpwanted on
{{
d:/mozbuild\mozilla\intl\uconv\ucvlatin\nsUnicodeToTSCII.cpp(277) : warning
C4244: '=' : conversion from 'PRUnichar' to 'char', possible loss of data
d:/mozbuild\mozilla\intl\uconv\ucvlatin\nsUnicodeToTSCII.cpp(301) : warning
C4244: '=' : conversion from 'PRUnichar' to 'char', possible loss of data

d:/mozbuild\mozilla\intl\uconv\src\nsTextToSubURI.cpp(231) : warning C4309:
'argument' : truncation of constant value
}}

Comment 11

[Serge Gautherie (:sgautherie)]

(Neil)
(In reply to comment #5)
> (From update of attachment 179177 [details] [diff] [review] [edit])
> Both MOZ_WIDGET_GTK and MOZ_WIDGET_GTK2 define MOZ_X11 so imho the real bug is
> that the line was put in the wrong place.

(timeless)
(In reply to comment #9)
> (From update of attachment 179272 [details] [diff] [review] [edit])
> Qt at the very least can happen in cases that are not x11.
> 
> theoretically gtk(2) can too, although i'm not sure how color maps fair wrt non
> x11.

I can leave Qt outside;

but now I'm confused about GTK(2): do we need something like Cv1 or like Cv2 ?

Comment 12

[[e-mail]]

Use different variable names in each #ifdef block perhaps?

Comment 13

[Serge Gautherie (:sgautherie)]

(In reply to comment #12)
> Use different variable names in each #ifdef block perhaps?

That would be a solution, but it was "rejected" in bug 258055, and I agree that
there should be no need for it.
I do believe that answering to whether X11 is guaranted to be defined when
GTK(2) is defined is enough it select either Cv1 or Cv2 like patch, is it not ?

Comment 14

[[e-mail]]

OK, we'd better go with Cv1 then...

Comment 15

[Serge Gautherie (:sgautherie)]

Attachment: (Cv3) <nsAppRunner.cpp> [Checkin: See comment 36]

"Self-Compiled [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b)
Gecko/20050324] (<-- v1.8b1 !) (W2Ksp4)"

Cv2, with comment 14 suggestion.


(Josh, could you update your unrecognized '[e-mail]' on
<http://www.mozilla.org/owners.html#XPApps> ?)

Comment 16

[Serge Gautherie (:sgautherie)]

"Self-Compiled [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b)
Gecko/20050324] (<-- v1.8b1 !) (W2Ksp4)"

Stuart, Tim:

To be fixed
{{
d:/mozbuild\mozilla\jpeg\jcmarker.c(658) : warning C4028: formal parameter 2
different from declaration
d:/mozbuild\mozilla\jpeg\jcmarker.c(659) : warning C4028: formal parameter 2
different from declaration
}}

The 2 functions have |int16|, while the forward declarations have |int|.

What would be the proper fix ?

(Tim, Could you update '[e-mail]' on
<http://www.mozilla.org/owners.html>, which doesn't match anything :-()

Comment 17

[Serge Gautherie (:sgautherie)]

"Self-Compiled [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b)
Gecko/20050324] (<-- v1.8b1 !) (W2Ksp4)"

Brendan:

To be fixed
{{
d:\mozbuild\mozilla\js\src\fdlibm\e_acos.c(117) : warning C4723: potential
divide by 0
d:\mozbuild\mozilla\js\src\fdlibm\e_asin.c(125) : warning C4723: potential
divide by 0
d:\mozbuild\mozilla\js\src\fdlibm\e_rem_pio2.c(204) : warning C4700: local
variable 'z' used without having been initialized
d:\mozbuild\mozilla\js\src\fdlibm\k_cos.c(125) : warning C4700: local variable
'qx' used without having been initialized
}}

The 2 |divide by 0| seem wanted !? (Is there a way to silence the compiler ??)
The 2 uninitialized variables seem to be fixable !?

(helpwanted)

Comment 18

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1) <nsprpub/*> (11/16)

"Self-Compiled [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b)
Gecko/20050324] (<-- v1.8b1 !) (W2Ksp4)"

Fixes
{{
d:/mozbuild\mozilla\nsprpub\pr\src\misc\prdtoa.c(1252) : warning C4554: '>>' :
check operator precedence for possible error; use parentheses to clarify
precedence
d:/mozbuild\mozilla\nsprpub\pr\src\misc\prdtoa.c(1254) : warning C4554: '<<' :
check operator precedence for possible error; use parentheses to clarify
precedence
d:/mozbuild\mozilla\nsprpub\pr\src\misc\prdtoa.c(1254) : warning C4554: '>>' :
check operator precedence for possible error; use parentheses to clarify
precedence
d:/mozbuild\mozilla\nsprpub\pr\src\misc\prdtoa.c(1259) : warning C4554: '>>' :
check operator precedence for possible error; use parentheses to clarify
precedence
d:/mozbuild\mozilla\nsprpub\pr\src\misc\prdtoa.c(1261) : warning C4554: '>>' :
check operator precedence for possible error; use parentheses to clarify
precedence
d:/mozbuild\mozilla\nsprpub\pr\src\misc\prdtoa.c(1338) : warning C4554: '<<' :
check operator precedence for possible error; use parentheses to clarify
precedence
d:/mozbuild\mozilla\nsprpub\pr\src\misc\prdtoa.c(1983) : warning C4554: '<<' :
check operator precedence for possible error; use parentheses to clarify
precedence
d:/mozbuild\mozilla\nsprpub\pr\src\misc\prdtoa.c(2871) : warning C4554: '<<' :
check operator precedence for possible error; use parentheses to clarify
precedence
d:/mozbuild\mozilla\nsprpub\pr\src\misc\prdtoa.c(2872) : warning C4554: '>>' :
check operator precedence for possible error; use parentheses to clarify
precedence
d:/mozbuild\mozilla\nsprpub\pr\src\misc\prdtoa.c(2872) : warning C4554: '<<' :
check operator precedence for possible error; use parentheses to clarify
precedence
d:/mozbuild\mozilla\nsprpub\pr\src\misc\prdtoa.c(3357) : warning C4102:
'trimzeros' : unreferenced label
}}
It seems that all the |precedence| warnings should not be triggered ('+'/'-'
are higher prioritized than '<<'/'>>', but here they are :-/

helpwanted on
{{
d:/mozbuild\mozilla\nsprpub\pr\src\io\priometh.c(368) : warning C4018: '<' :
signed/unsigned mismatch

d:/mozbuild\mozilla\nsprpub\pr\src\misc\prdtoa.c(2372) : warning C4244: '=' :
conversion from 'double' to 'PRUint32', possible loss of data
d:/mozbuild\mozilla\nsprpub\pr\src\misc\prdtoa.c(2625) : warning C4018: '<=' :
signed/unsigned mismatch
d:/mozbuild\mozilla\nsprpub\pr\src\misc\prdtoa.c(3015) : warning C4244: '=' :
conversion from 'double' to 'PRInt32', possible loss of data

d:/mozbuild\mozilla\nsprpub\pr\src\threads\prtpd.c(178) : warning C4018: '>=' :
signed/unsigned mismatch
}}

Comment 19

[Brendan Eich [:brendan]]

The e_cos.c and e_sin.c warnings are bogus -- that code wants to synthesize a NaN.

The uninitialized warnings are more serious.  The e_rem_pio2.c one seems like a
typo, or something bad.  It should be fixed.  The k_cos.c one looks easier to fix.

I would appreciate a new, focused bug being filed on just these fdlibm warnings.

/be

Comment 20

[Serge Gautherie (:sgautherie)]

"Self-Compiled [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b)
Gecko/20050324] (<-- v1.8b1 !) (W2Ksp4)"

David:

To be fixed
{{
d:\mozbuild\mozilla\layout\generic\nsframeframe.cpp(636) : warning C4715:
'ConvertOverflow' : not all control paths return a value
}}
at <http://lxr.mozilla.org/mozilla/source/layout/generic/nsFrameFrame.cpp#626>

I guess the compiler would like some kind of default case, with a return
statement !?

(helpwanted)

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Those are all the valid values, so there should just be an NS_NOTREACHED at the
end of the function, perhaps with a return nsIScrollable::Scrollbar_Auto;.

Comment 22

[Serge Gautherie (:sgautherie)]

(In reply to comment #19)
> I would appreciate a new, focused bug being filed on just these fdlibm warnings.

I filed bug 288993.

Comment 23

[timeless]

(In reply to comment #15)
> (Josh, could you update your unrecognized '[e-mail]' on
> <http://www.mozilla.org/owners.html#XPApps> ?)

serge: owners.html contains a list of *email* addresses for cvs accounts. they
do not have to match bugzilla accounts.

fwiw tor is [e-mail] in bugzilla.

Comment 24

[Serge Gautherie (:sgautherie)]

Attachment: (Fv1) <nsframeframe.cpp>

"Self-Compiled [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b)
Gecko/20050324] (<-- v1.8b1 !) (W2Ksp4)"

Fixes
{{
d:\mozbuild\mozilla\layout\generic\nsframeframe.cpp(636) : warning C4715:
'ConvertOverflow' : not all control paths return a value
}}
per comment 21.

Comment 25

[Serge Gautherie (:sgautherie)]

Comment on attachment 179280 [details] [diff] [review]
(Dv1a) <intl/*> (4/7)
[Checked in: Comment 25]

Check in: { 2005-04-05 01:31	neil%parkwaycc.co.uk }

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 179705 [details] [diff] [review]
(Fv1) <nsframeframe.cpp>

>+      NS_NOTREACHED("Invalid aOverflow parameter; returning Scrollbar_Auto as default.");
>+      return nsIScrollable::Scrollbar_Auto;

remove the text "; returning ...default.", and lowercase the "I".

> }
> 
>-

If you're bothering, remove both blank lines (but not the newline after the
brace, since LF on Unix is a line terminator rather than a line separator).

r+sr=dbaron with those changes

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 178944 [details] [diff] [review]
(Bv1) <nsXPInstallManager.cpp>
[Checked in: Comment 30]

r/sr=dveditz

Comment 28

[Serge Gautherie (:sgautherie)]

Comment on attachment 178944 [details] [diff] [review]
(Bv1) <nsXPInstallManager.cpp>
[Checked in: Comment 30]

'approval1.8b4=?': (SeaMonkey only !?)
Trivial Installer nit, no risk.

Comment 29

[Mark Banner (:standard8)]

Comment on attachment 178944 [details] [diff] [review]
(Bv1) <nsXPInstallManager.cpp>
[Checked in: Comment 30]

I just checked this in on behalf of Serge:
/cvsroot/mozilla/xpinstall/src/nsXPInstallManager.cpp,v  <-- 
nsXPInstallManager.cpp
new revision: 1.134; previous revision: 1.133

Comment 30

[Serge Gautherie (:sgautherie)]

(In reply to comment #29)
> (From update of attachment 178944 [details] [diff] [review] [edit])

Then, actually in
{{
2005-07-30 13:10	bugzilla%standard8.demon.co.uk 	mozilla/ xpinstall/ src/
nsXPInstallManager.cpp 	1.135
}}

Comment 31

[Serge Gautherie (:sgautherie)]

Attachment: (Fv1a) <nsframeframe.cpp> [Checkin: See comment 32]

"Self-Compiled [Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b)
Gecko/20050324] (<-- v1.8b1 !) (W2Ksp4)"

Fv1, with comment 26 suggestions.

Keeping
{{
(Fv1) <nsframeframe.cpp>	 patch		2005-04-05 07:11 PDT	1.82 KB
	dbaron: review+
dbaron: superreview+ 
}}

'approval1.8b4=?':
Trivial warning nit, no risk.

Comment 32

[Serge Gautherie (:sgautherie)]

Comment on attachment 191705 [details] [diff] [review]
(Fv1a) <nsframeframe.cpp>
[Checkin: See comment 32]

NeilR found that this patch is "Duplicate of bug 262917":
it seems I missed that bug report in the first place,
then it was fixed there before I processed DavidB's r+/sr+ associated comment
here :-<

Comment 33

[Serge Gautherie (:sgautherie)]

Comment on attachment 178486 [details] [diff] [review]
(Av1) <mozInlineSpellChecker.cpp> (3/4)
[Checkin: See comment 33]

Theses were fixed in the meantime by (later) bug 292520.

Comment 34

[timeless]

Comment on attachment 179574 [details] [diff] [review]
(Cv3) <nsAppRunner.cpp>
[Checkin: See comment 36]

since you're changing this, and someone's going to get blame, the proper way to
use PR_GetEnv involves:

const char* foo = PR_GetEnv("BAR");
(foo && *foo)

not simply (foo).

the only way this should happen is if code on certain platforms decides to
unset the var, but it'd be nice if we did the right thing.

Comment 35

[Serge Gautherie (:sgautherie)]

Comment on attachment 179574 [details] [diff] [review]
(Cv3) <nsAppRunner.cpp>
[Checkin: See comment 36]

'approval1.8b4=?': (SeaMonkey only)
Trivial Bootstrap nit, no risk.

Comment 36

[timeless]

Comment on attachment 179574 [details] [diff] [review]
(Cv3) <nsAppRunner.cpp>
[Checkin: See comment 36]

mozilla/xpfe/bootstrap/nsAppRunner.cpp	1.443

the patch doesn't apply straight to trunk, so if you really want it for branch
(and i don't see why), please post a diff from 1.442 to 1.443

Comment 37

[Serge Gautherie (:sgautherie)]

(In reply to comment #34)
> (From update of attachment 179574 [details] [diff] [review] [edit])
> since you're changing this, and someone's going to get blame, the proper way to
> use PR_GetEnv involves:

Issue moved to bug 306840.

Comment 38

[Serge Gautherie (:sgautherie)]

Comment on attachment 179591 [details] [diff] [review]
(Ev1) <nsprpub/*> (11/16)


No super-review from <[e-mail]> since "2005-04-04" :-(

Comment 39

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1a) <nsprpub/*> (10/13) [Moved to bug 508531]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090518 SeaMonkey/2.0b1pre] (experimental/_m-c_, home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/a7c0b3588242
 +http://hg.mozilla.org/comm-central/rev/6786ebf24275 + bug 493008 patches)

Ev1, (trivially) unbirotted and "hg"...

Keeping
{
bryner: review+
darin.moz: superreview+
}


(In reply to comment #18)
> d:/mozbuild\mozilla\nsprpub\pr\src\misc\prdtoa.c(3357) : warning C4102:
> 'trimzeros' : unreferenced label

This warning was fixed by
http://hg.mozilla.org/mozilla-central/rev/956071116564

Comment 40

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 379347 [details] [diff] [review]
(Ev1a) <nsprpub/*> (10/13)
[Moved to bug 508531]

Serge, 
NSPR operates under different rules than some other parts of the tree.  
The master repository for NSPR is still CVS.  r+ and sr+ reviews can 
only be given by NSPR modules owners and peers, and others who are 
specifically invited to do reviews by the module owners.  The module 
owners only review patches in NSPR bugs (that is, bugs whose product 
is NSPR).  Patches for NSPR bugs must be CVS patches, not Hg patches.  
r+ and sr+ reviews given to old NSPR patches may not be "carried forward" 
to new & different patches.  

So, if you want NSPR changed, file an NSPR bug, and attach a CVS diff -pu
patch to it, and ask one of the NSPR module owners/peers to review.  

Having said that, I will add that there are some files in NSPR that are
actually copies of files written by third parties (used with permission)
and the NSPR module owners try to keep them as exact copies of the files
from those third party authors, so that new patches from those authors 
apply cleanly.  prdtoa.c is one of those files.  See bug 439144 comment 8.
Despite that, there have been two recent exceptions to that rule, for
Bug 439144 and Bug 449665, so it won't hurt to file another bug and ask,
but don't be surprised if that bug gets resolved WONTFIX.

Comment 41

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 379347 [details] [diff] [review]
(Ev1a) <nsprpub/*> (10/13)
[Moved to bug 508531]

Oh, one more thing.  NSPR is c code, not c++, and the NSPR module owners 
resist attempts to make changes to silence c++ compiler warnings that are 
not also c compiler warnings.  So, if this patch is fixing c++ warnings 
that are not also c warnings, I promise you it won't fly.

Comment 42

[Serge Gautherie (:sgautherie)]

(In reply to comment #40)
> NSPR operates under different rules than some other parts of the tree.  

Thanks for the reminder.

> Patches for NSPR bugs must be CVS patches, not Hg patches.

Sorry: I am trying to re-learn cvs,
but (atm) Hg (mq) seems so much easier to manage patches (queues)!

(NB: svn or hg mirror, anyone?)

> So, if you want NSPR changed, file an NSPR bug

I filed bug 508531.

Comment 43

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 17, 
> Is there a way to silence the compiler ??
Yes, it's easy to silence the MSVC compiler for specific types of warnings.
Command line option -wdNNNN will silence warnings of type NNNN.
As many of these as desired can be added to the command line.

Warnings can be silenced for specific regions of code with the insertion of 
#pragma directives.  But this is not encouraged in NSPR because it requires
so much ifdefing.

#pragma warning( push )
#pragma warning( disable: 4700 4701) /* disables warnings C4700 and C4701 */
     some code that would cause those warnings
#pragma warning( pop )

Comment 44

[Robert Kaiser]

compiler warnings are build config, let's put this there.

Comment 45

[Justin Wood (:Callek)]

Not touched for a long while, use new bug if we need more, so it falls off my "Might need a checkin" tracker