228780 - Fix all "Unused variable '[...]'" 'Build Warnings' (Part 2/2: "with function call")

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Serge Gautherie (:sgautherie)]

This bug goes along bug 90906.

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) <nsMacMain.cpp>

Fix for:
{
/builds/tinderbox/Mozilla1.6/Darwin_6.6_Clobber/mozilla/webshell/tests/viewer/nsMacMain.cpp:
In
   function `int main(int, char**)':
/builds/tinderbox/Mozilla1.6/Darwin_6.6_Clobber/mozilla/webshell/tests/viewer/nsMacMain.cpp:450:
warning: unused
   variable `OSErr err'
}

Comment 2

[Serge Gautherie (:sgautherie)]

Comment on attachment 137724 [details] [diff] [review]
(Av1) <nsMacMain.cpp>

'r=?': (see comment 1)
I have no compiler: Can you compile/test/review it ? Thanks.

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Av1b) <nsMacMain.cpp>

Patch Av1, more explicit.

Comment 4

[Serge Gautherie (:sgautherie)]

Comment on attachment 137748 [details] [diff] [review]
(Av1b) <nsMacMain.cpp>


'r=?': (see comment 1)
I have no compiler: Can you compile/test/review it ? Thanks.

Comment 5

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) <morkParser.cpp>

Solve
{ (bienvenu)
83.	db/mork/src/morkParser.cpp:1375 (See build log excerpt)
	Unused variable `nsresult rv'

1373 mork_pos here;
1374 nsIMdbEnv *ev = mev->AsMdbEnv();
1375 nsresult rv= mParser_Stream->Tell(ev, &here);
1376 if ( here > 0 ) // positive?
1377   --here;
}

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 139057 [details] [diff] [review]
(Bv1) <morkParser.cpp>

I have no compiler: Could you compile/test/review it ? Thanks.

Comment 7

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 137748 [details] [diff] [review]
(Av1b) <nsMacMain.cpp>

r=pink

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 137748 [details] [diff] [review]
(Av1b) <nsMacMain.cpp>

The explicit |(void)| cast should not be needed.  sr=bryner if you remove that.

Comment 9

[Serge Gautherie (:sgautherie)]

Attachment: (Av1c) <nsMacMain.cpp> [Checked in: Comment 13]

Av1b, with comment 8 suggestion(s):

*Removal of the explicit |(void)| cast, which was only there for human reader.
*Line up the next two lines of parameters, previously untouched.

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 141227 [details] [diff] [review]
(Av1c) <nsMacMain.cpp>
[Checked in: Comment 13]


Keeping
{
(Av1b) <nsMacMain.cpp>		 patch		2003-12-20 05:33 PST   
pinkerton: review+
bryner: superreview+
}

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 141227 [details] [diff] [review]
(Av1c) <nsMacMain.cpp>
[Checked in: Comment 13]


'approval1.7a?': trivial warning fix, no risk.

Comment 12

[chris hofmann]

Comment on attachment 141227 [details] [diff] [review]
(Av1c) <nsMacMain.cpp>
[Checked in: Comment 13]

a=chofmann

Comment 13

[Serge Gautherie (:sgautherie)]

Comment on attachment 141227 [details] [diff] [review]
(Av1c) <nsMacMain.cpp>
[Checked in: Comment 13]


Check in: { 02/12/2004 09:11	neil%parkwaycc.co.uk	mozilla/ webshell/
tests/ viewer/ nsMacMain.cpp	     1.55 }

Comment 14

[chris hofmann]

marking fixed

Comment 15

[[e-mail]]

Comment on attachment 139057 [details] [diff] [review]
(Bv1) <morkParser.cpp>

const nsresult rv isn't mozilla style even if the variable only gets used once.

Comment 16

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1b-r) <morkParser.cpp> (for review only)

Bv1, with comment 15 suggestion(s);
and without additional white-space cleanup.

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 146639 [details] [diff] [review]
(Bv1b-r) <morkParser.cpp> (for review only)


I have no compiler: Can you compile/test/review it ? Thanks.

Comment 18

[[e-mail]]

Comment on attachment 146639 [details] [diff] [review]
(Bv1b-r) <morkParser.cpp> (for review only)

Looks good to me, it seems more consistent with End/StartSpanOnThisByte.

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 146639 [details] [diff] [review]
(Bv1b-r) <morkParser.cpp> (for review only)

No super-review from <[e-mail]> since '2004-04-26' :-(

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 146639 [details] [diff] [review]
(Bv1b-r) <morkParser.cpp> (for review only)

rs=me, I'm trusting you guys, patch looks ok from the context diff.

/be

Comment 21

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1b-ci) <morkParser.cpp> (for check-in) [Checked in: Comment 23]

Bv1b-r, with full white-space diff.

Comment 22

[David :Bienvenu]

Comment on attachment 146639 [details] [diff] [review]
(Bv1b-r) <morkParser.cpp> (for review only)

moa=bienvenu

Comment 23

[Serge Gautherie (:sgautherie)]

Comment on attachment 151402 [details] [diff] [review]
(Bv1b-ci) <morkParser.cpp> (for check-in)
[Checked in: Comment 23]


Check in: { 2004-06-22 09:06	neil%parkwaycc.co.uk	mozilla/ db/ mork/ src/
morkParser.cpp	1.23 }

Comment 24

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) <nsMsgBodyHandler.cpp>

Fix for
{{
bienvenu

1.	mailnews/base/search/src/nsMsgBodyHandler.cpp:133 (See build log
excerpt)
	Unused variable `nsresult rv'

131 {
132   nsCOMPtr <nsIInputStream> inputStream;
133   nsresult rv = m_scope->GetInputStream(getter_AddRefs(inputStream));
134   if (inputStream)
135   {
}}

Comment 25

[Serge Gautherie (:sgautherie)]

Matthias:
I'm CC'ing you;
note that for some time, my primary ISP STMP service has been enable to send you
my messages... :-(

Comment 26

[Serge Gautherie (:sgautherie)]

Comment on attachment 151481 [details] [diff] [review]
(Cv1) <nsMsgBodyHandler.cpp>


Matthias: Could you try to compile, and possibly test, this patch ? Thanks.

Note
{{
GetInputStream

Defined as a function in:

    * mailnews/base/search/src/nsMsgSearchTerm.cpp, line 1464, as member of
class nsMsgSearchScopeTerm 
}}

Comment 27

[Matthias Bockelkamp]

Comment on attachment 151481 [details] [diff] [review]
(Cv1) <nsMsgBodyHandler.cpp>

Compiles and works.

Comment 28

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) <nsViewManager.cpp> [Checked in: Comment 45]

Fix for
{{
roc+

22.	view/src/nsViewManager.cpp:2332 (See build log excerpt)
	Unused variable `nsresult rv'

2330 nsCOMPtr<nsIWidget> parentWidget = getter_AddRefs(widget->GetParent());
2331 if (parentWidget.get() != aNewWidget) {
2332   nsresult rv = widget->SetParent(aNewWidget);
2333   NS_ASSERTION(NS_SUCCEEDED(rv), "SetParent failed!");
2334 }
}} (LineNo is now 2353.)

due to
{{
3.323 <[e-mail]> 2004-04-07 08:22
Bug 235897. hidden widgets don't count as part of the opque region.
r+sr=dbaron,a=mkaply
}} (This is not in the patch attached to that bug: added when checked in.)

Comment 29

[Serge Gautherie (:sgautherie)]

Comment on attachment 151802 [details] [diff] [review]
(Dv1) <nsViewManager.cpp>
[Checked in: Comment 45]

Matthias: Could you try to compile, and possibly test, this patch ? Thanks.

Comment 30

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1) <xpcom/tests/*.cpp>

Fix for 9 |Unused variable `{...}'|,
blamed at warren (20, 21), mjudge (5-10), jband (7);
plus a few code rewrites.

Comment 31

[Serge Gautherie (:sgautherie)]

Comment on attachment 151818 [details] [diff] [review]
(Ev1) <xpcom/tests/*.cpp>


Matthias: Could you try to compile, and possibly test, this patch ? Thanks.

Doug:

Comment 32

[Matthias Bockelkamp]

Comment on attachment 151802 [details] [diff] [review]
(Dv1) <nsViewManager.cpp>
[Checked in: Comment 45]

Compiles and works.

Comment 33

[Matthias Bockelkamp]

Comment on attachment 151818 [details] [diff] [review]
(Ev1) <xpcom/tests/*.cpp>

What must I do to compile these files? My "make -f client.mk build" doesn't do
that.

Comment 34

[Serge Gautherie (:sgautherie)]

Comment on attachment 151818 [details] [diff] [review]
(Ev1) <xpcom/tests/*.cpp>

No super-review from <[e-mail]> since "2004-06-27" :-(

I have no compiler (and Matthias can't): Could you compile/test/review this
patch ? Thanks.

Comment 35

[Serge Gautherie (:sgautherie)]

Comment on attachment 151481 [details] [diff] [review]
(Cv1) <nsMsgBodyHandler.cpp>

No super-review from <[e-mail]> since "2004-06-22" :-(

Comment 36

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 151481 [details] [diff] [review]
(Cv1) <nsMsgBodyHandler.cpp>

> void nsMsgBodyHandler::OpenLocalFolder()
> {
>   nsCOMPtr <nsIInputStream> inputStream;
>   nsresult rv = m_scope->GetInputStream(getter_AddRefs(inputStream));
>-  if (inputStream)
>+  if (NS_SUCCEEDED(rv) && inputStream)
>   {
> 

Is there ever a legimate case where NS_SUCCEEDED(rv) could return true and
inputStream could be null?  If not, a better fix would be something along the
lines of:

> if (NS_SUCCEEDED(rv))
> {
>   NS_ASSERTION(inputStream, "NULL inputStream returned")

Comment 37

[Serge Gautherie (:sgautherie)]

(In reply to comment #36)
> (From update of attachment 151481 [details] [diff] [review])
> Is there ever a legimate case where NS_SUCCEEDED(rv) could return true and
> inputStream could be null?

I wouldn't know: CC'ing bienvenu (as blamed author) !
Or I could simply prepare a patch your way ?

Comment 38

[David :Bienvenu]

no, if there's no input stream, an error should be returned. Adding an extra
check is just code bloat.

Comment 39

[Serge Gautherie (:sgautherie)]

(In reply to comment #38)
> no, if there's no input stream, an error should be returned. Adding an extra
> check is just code bloat.

Then, should I go for the assertion, or can I simply get rid of the inputStream
test (== _replaced_ by rv test only) ?

Comment 40

[Dan Mosedale (:dmosedale, :dmose)]

Serge: sounds like either way would be fine.  Might as well just replace.

Comment 41

[Serge Gautherie (:sgautherie)]

Attachment: (Cv2) <nsMsgBodyHandler.cpp> [Checked in: Comment 44]

Cv1, with comment 40 suggestions.

Comment 42

[Serge Gautherie (:sgautherie)]

Comment on attachment 156855 [details] [diff] [review]
(Cv2) <nsMsgBodyHandler.cpp>
[Checked in: Comment 44]

Keeping
{{
(Cv1) <nsMsgBodyHandler.cpp>	 patch		2004-06-22 15:20 PDT   
neil.parkwaycc.co.uk: review+ 
}}

Comment 43

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 156855 [details] [diff] [review]
(Cv2) <nsMsgBodyHandler.cpp>
[Checked in: Comment 44]

sr=dmose

Comment 44

[Serge Gautherie (:sgautherie)]

Comment on attachment 156855 [details] [diff] [review]
(Cv2) <nsMsgBodyHandler.cpp>
[Checked in: Comment 44]


Check in: { 2004-08-24 03:14	neil%parkwaycc.co.uk	mozilla/ mailnews/
base/ search/ src/ nsMsgBodyHandler.cpp      1.27 }

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 151802 [details] [diff] [review]
(Dv1) <nsViewManager.cpp>
[Checked in: Comment 45]

checked in too.

Comment 46

[Serge Gautherie (:sgautherie)]

Comment on attachment 151818 [details] [diff] [review]
(Ev1) <xpcom/tests/*.cpp>

No review from <[e-mail]> since "2004-08-23" :-(

I have no compiler (and Matthias can't): Could you compile/test/review this
patch ? Thanks.

Comment 47

[Darin Fisher]

Comment on attachment 151818 [details] [diff] [review]
(Ev1) <xpcom/tests/*.cpp>

>Index: mozilla/xpcom/tests/TestArray.cpp

>+#ifdef DEBUG
>+  nsresult rv =
>+#endif
>+    aArray->Count(&cnt);
>   NS_ASSERTION(NS_SUCCEEDED(rv), "Count failed");

hmm... the point of these tests seems to be to check that
things are functioning correctly.  perhaps the problem here
is that NS_ASSERTION is used to do the checking.  perhaps
in a non-debug build it would be useful to do error checking
as well.  i think i'd change this to always check |rv| and
then output an error message such that even in a non-debug
build the user would see a problem being reported.  since
stdout is disabled in Win32 non-debug builds, one good soln
is to use NSPR logging (that's what i do in netwerk/test).


>+static void Check(const char * const s1, const char * const s2, const PRIntn n)
> {
>+#ifdef DEBUG
>+  PRIntn clib =
>+#endif
>+    PL_strcmp(s1, s2);
>+#ifdef DEBUG
>+  PRIntn clib_n =
>+#endif
>+    PL_strncmp(s1, s2, n);
>+/*
>   PRIntn clib_case = PL_strcasecmp(s1, s2);
>   PRIntn clib_case_n = PL_strncasecmp(s1, s2, n);
>+*/

why comment out this code?  should it be removed instead?


>+#ifdef DEBUG
>+  PRIntn u2 =
>+#endif
>+    nsCRT::strcmp(us1, us2);
>+#ifdef DEBUG
>+  PRIntn u2_n =
>+#endif
>+    nsCRT::strncmp(us1, us2, n);
> 
>   NS_ASSERTION(sign(clib) == sign(u2), "strcmp");
>   NS_ASSERTION(sign(clib_n) == sign(u2_n), "strncmp");
> }

so, the whole point of this function seems to be to perform
the assertion.	so, what's the point of compiling this code
in non-debug builds?  or better yet, why not change the code
as i suggested above so that it is useful in a non-debug build?

the same problem occurs throughout this patch.	the only thing
the patch does is silence compiler warnings in non-debug builds.
you could do the same by using --disable-tests in your mozconfig.
perhaps the tests should be made to work in non-debug builds
instead.

Comment 48

[Robert Kaiser]

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

Comment 49

[Chris Peterson [:cpeterson]]

All -Wunused-variable warnings have been fixed. \o/