Closed Bug 287540 Opened 15 years ago Closed 9 years ago

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

Categories

(SeaMonkey :: Build Config, defect, trivial)

x86
Windows 2000
defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sgautherie, Assigned: sgautherie)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 6 obsolete files)

1.28 KB, patch
Details | Diff | Splinter Review
461 bytes, patch
dveditz
: review+
dveditz
: superreview+
benjamin
: approval1.8b4+
Details | Diff | Splinter Review
2.66 KB, patch
smontagu
: review+
blizzard
: superreview+
Details | Diff | Splinter Review
2.01 KB, patch
timeless
: review+
Details | Diff | Splinter Review
1.83 KB, patch
sgautherie
: review+
sgautherie
: superreview+
benjamin
: approval1.8b4+
Details | Diff | Splinter Review
 
<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.
Assignee: general → gautheri
"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)
Attachment #178486 - Flags: superreview?(mscott)
Attachment #178486 - Flags: review?(mscott)
Status: NEW → ASSIGNED
"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
}}
Attachment #178944 - Flags: superreview?(dveditz)
Attachment #178944 - Flags: review?(dveditz)
Attached patch (Cv1) <nsAppRunner.cpp> (obsolete) — Splinter Review
"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
}}
Attachment #179177 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #179177 - Flags: review?(neil.parkwaycc.co.uk)
OS: All → Windows 2000
Summary: Fix compiler warnings: all "Comparison between signed and unsigned integer expressions" → Fix various compiler warnings from my Windows non-debug build
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.
Attachment #179177 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #179177 - Flags: superreview-
Attachment #179177 - Flags: review?(neil.parkwaycc.co.uk)
(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
}}
Depends on: 258055
Attached patch (Cv2) <nsAppRunner.cpp> (obsolete) — Splinter Review
"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.
Attachment #179177 - Attachment is obsolete: true
Attachment #179272 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #179272 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch (Dv1) <intl/*> (1/7) (obsolete) — Splinter Review
"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
}}
Attachment #179275 - Flags: superreview?(blizzard)
Attachment #179275 - Flags: review?(smontagu)
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.
Attachment #179272 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #179272 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #179272 - Flags: review-
"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
}}
Attachment #179275 - Attachment is obsolete: true
Attachment #179280 - Flags: superreview?(blizzard)
Attachment #179280 - Flags: review?(smontagu)
Attachment #179275 - Attachment description: (Dv1) <nsLanguageAtomService.cpp> → (Dv1) <intl/*> (1/6)
Attachment #179275 - Flags: superreview?(blizzard)
Attachment #179275 - Flags: review?(smontagu)
(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 ?
Use different variable names in each #ifdef block perhaps?
(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 ?
Attachment #179280 - Flags: review?(smontagu) → review+
OK, we'd better go with Cv1 then...
"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 'timeless@mozdev.org' on
<http://www.mozilla.org/owners.html#XPApps> ?)
Attachment #179272 - Attachment is obsolete: true
Attachment #179574 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #179574 - Flags: review?(timeless)
Attachment #179280 - Flags: superreview?(blizzard) → superreview+
"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 'tor@cs.brown.edu' on
<http://www.mozilla.org/owners.html>, which doesn't match anything :-()
"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)
Attached patch (Ev1) <nsprpub/*> (11/16) (obsolete) — Splinter Review
"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
}}
Attachment #179591 - Flags: superreview?(darin)
Attachment #179591 - Flags: review?(darin)
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
"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)
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;.
Depends on: 288993
(In reply to comment #19)
> I would appreciate a new, focused bug being filed on just these fdlibm warnings.

I filed bug 288993.
(In reply to comment #15)
> (Josh, could you update your unrecognized 'timeless@mozdev.org' 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 tor@acm.org in bugzilla.
Attached patch (Fv1) <nsframeframe.cpp> (obsolete) — Splinter Review
"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.
Attachment #179705 - Flags: superreview?(dbaron)
Attachment #179705 - Flags: review?(dbaron)
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 }
Attachment #179280 - Attachment description: (Dv1a) <intl/*> (3/6) → (Dv1a) <intl/*> (4/7) [Checked in: Comment 25]
Attachment #179280 - Attachment is obsolete: true
Attachment #179275 - Attachment description: (Dv1) <intl/*> (1/6) → (Dv1) <intl/*> (1/7)
Summary: Fix various compiler warnings from my Windows non-debug build → Fix various _C++_ compiler warnings from my Windows non-debug build
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
Attachment #179705 - Flags: superreview?(dbaron)
Attachment #179705 - Flags: superreview+
Attachment #179705 - Flags: review?(dbaron)
Attachment #179705 - Flags: review+
Comment on attachment 178944 [details] [diff] [review]
(Bv1) <nsXPInstallManager.cpp>
[Checked in: Comment 30]

r/sr=dveditz
Attachment #178944 - Flags: superreview?(dveditz)
Attachment #178944 - Flags: superreview+
Attachment #178944 - Flags: review?(dveditz)
Attachment #178944 - Flags: review+
Comment on attachment 178944 [details] [diff] [review]
(Bv1) <nsXPInstallManager.cpp>
[Checked in: Comment 30]

'approval1.8b4=?': (SeaMonkey only !?)
Trivial Installer nit, no risk.
Attachment #178944 - Flags: approval1.8b4?
Attachment #178944 - Flags: approval1.8b4? → approval1.8b4+
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
Attachment #178944 - Attachment description: (Bv1) <nsXPInstallManager.cpp> → (Bv1) <nsXPInstallManager.cpp> [Checked in]
Attachment #178944 - Attachment description: (Bv1) <nsXPInstallManager.cpp> [Checked in] → (Bv1) <nsXPInstallManager.cpp> [Checked in: Comment 30]
Attachment #178944 - Attachment is obsolete: true
(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
}}
"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.
Attachment #179705 - Attachment is obsolete: true
Attachment #191705 - Flags: superreview+
Attachment #191705 - Flags: review+
Attachment #191705 - Flags: approval1.8b4?
Attachment #191705 - Flags: approval1.8b4? → approval1.8b4+
Depends on: 262917
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 :-<
Attachment #191705 - Attachment is obsolete: true
Attachment #191705 - Attachment description: (Fv1a) <nsframeframe.cpp> → (Fv1a) <nsframeframe.cpp> [Checkin: See comment 32]
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.
Attachment #178486 - Attachment description: (Av1) <mozInlineSpellChecker.cpp> (3/4) → (Av1) <mozInlineSpellChecker.cpp> (3/4) [Checkin: See comment 33]
Attachment #178486 - Attachment is obsolete: true
Attachment #178486 - Flags: superreview?(mscott)
Attachment #178486 - Flags: review?(mscott)
Depends on: 292520
Attachment #179574 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
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.
Attachment #179574 - Flags: review?(timeless) → review+
Comment on attachment 179574 [details] [diff] [review]
(Cv3) <nsAppRunner.cpp>
[Checkin: See comment 36]

'approval1.8b4=?': (SeaMonkey only)
Trivial Bootstrap nit, no risk.
Attachment #179574 - Flags: approval1.8b4?
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
Attachment #179574 - Attachment is obsolete: true
Attachment #179574 - Flags: approval1.8b4?
Attachment #179574 - Attachment description: (Cv3) <nsAppRunner.cpp> → (Cv3) <nsAppRunner.cpp> [Checkin: See comment 36]
(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 on attachment 179591 [details] [diff] [review]
(Ev1) <nsprpub/*> (11/16)


No super-review from <darin@meer.net> since "2005-04-04" :-(
Attachment #179591 - Flags: review?(darin) → review?(bryner)
Attachment #179591 - Flags: review?(bryner) → review+
Attachment #179591 - Flags: superreview?(darin) → superreview+
[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
Attachment #179591 - Attachment is obsolete: true
Attachment #379347 - Flags: superreview+
Attachment #379347 - Flags: review+
Attachment #178486 - Attachment is obsolete: false
Attachment #178944 - Attachment is obsolete: false
Attachment #191705 - Attachment is obsolete: false
Attachment #179574 - Attachment is obsolete: false
Attachment #179280 - Attachment is obsolete: false
Keywords: checkin-needed
Whiteboard: [c-n: Ev1a to cvs/1.9.0]
Attachment #379347 - Flags: superreview+
Attachment #379347 - Flags: review+
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 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.
Depends on: 508531
Attachment #379347 - Attachment description: (Ev1a) <nsprpub/*> (10/13) → (Ev1a) <nsprpub/*> (10/13) [Moved to bug 508531]
Attachment #379347 - Attachment is obsolete: true
No longer depends on: 508531
(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.
Keywords: checkin-needed
Whiteboard: [c-n: Ev1a to cvs/1.9.0]
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 )
compiler warnings are build config, let's put this there.
Component: General → Build Config
QA Contact: general → build-config
Not touched for a long while, use new bug if we need more, so it falls off my "Might need a checkin" tracker
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.