Reduce the number of warnings while compiling Mozilla

VERIFIED FIXED in mozilla1.3alpha

Status

MailNews Core
Backend
--
trivial
VERIFIED FIXED
15 years ago
9 years ago

People

(Reporter: Pratik, Assigned: dmose)

Tracking

Trunk
mozilla1.3alpha
All
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

15 years ago
I'm not sure where this bug belongs. I want to reduce the number of warnings
during the compilation of Mozilla. Looking at the list I see a whole bunch of
"Unused symbols". I've fixed the straightforward "Unused variable" warnings in
the mailnews directory (hence filing this bug under mailnews). Will attach a
patch shortly.
(Reporter)

Comment 1

15 years ago
Created attachment 105999 [details] [diff] [review]
Fixes many of the unused variable warnings
(Reporter)

Comment 2

15 years ago
CCing people whose warnings this patch fixes.

Updated

15 years ago
QA Contact: gayatri → putterman

Updated

15 years ago
Keywords: patch

Comment 3

15 years ago
-  nsresult rv = GetRootFolder(getter_AddRefs(rootFolder));
+  GetRootFolder(getter_AddRefs(rootFolder));
   nsXPIDLCString rootURI;

we shouldn't blow off this error - we should have
NS_ENSURE_SUCCESS(rv, rv); after the GetRootFolder call.

In the other places where you're removing the rv = part, could you put (void) in
front of the call so we know we're explicitly ignoring the error?

Thx for fixing these.
(Reporter)

Comment 4

15 years ago
bienvenu,

Thanks for your comments. I'll post an updated patch tomorrow (hopefully).
(Reporter)

Comment 5

15 years ago
Created attachment 106085 [details] [diff] [review]
v2 of patch

This addresses bienvenu's comments.
Attachment #105999 - Attachment is obsolete: true
R=ducarroz for changes in mime and smime

Comment 7

15 years ago
thx for looking at these.

nsSmtpService::GetSmtpServerByIdentity(nsIMsgIdentity *aSenderIdentity,
nsISmtpServer **aSmtpServer)
 {
   NS_ENSURE_ARG_POINTER(aSmtpServer);
-  nsresult rv;
+  nsresult rv = PR_FALSE;

PR_FALSE is not a valid rv (sorry if I missed this last time). You can use
NS_ERROR_FAILURE, and maybe add a comment to the effect that this error is not
ever returned directly. 

could you just remove this?
-  char * htmlLine2 = NULL;
+//  char * htmlLine2 = NULL;

oh, and one last thing - if you're going to change the PR_Asserts, could you
change them to NS_ASSERTION? PR_Asserts abort execution in debug builds, which
is a royal pain.
(Reporter)

Comment 8

15 years ago
>nsSmtpService::GetSmtpServerByIdentity(nsIMsgIdentity *aSenderIdentity,
>nsISmtpServer **aSmtpServer)
> {
>   NS_ENSURE_ARG_POINTER(aSmtpServer);
>-  nsresult rv;
>+  nsresult rv = PR_FALSE;
>
>PR_FALSE is not a valid rv (sorry if I missed this last time). You can use
>NS_ERROR_FAILURE, and maybe add a comment to the effect that this error is not
>ever returned directly. 

Take a look at the code

nsSmtpService::GetSmtpServerByIdentity(nsIMsgIdentity *aSenderIdentity,
nsISmtpServer **aSmtpServer)
{
  NS_ENSURE_ARG_POINTER(aSmtpServer);
  nsresult rv = NS_ERROR_FAILURE;

  // First try the identity's preferred server
  if (aSenderIdentity) {
      nsXPIDLCString smtpServerKey;
      rv = aSenderIdentity->GetSmtpServerKey(getter_Copies(smtpServerKey));
      if (NS_SUCCEEDED(rv) && !(smtpServerKey.IsEmpty()))
          rv = GetServerByKey(smtpServerKey, aSmtpServer);
  }

  // Fallback to the default
  if (NS_FAILED(rv) || !(*aSmtpServer))
      rv = GetDefaultServer(aSmtpServer);
  return rv;
}

If control falls through the first if then you're doing NS_FAILED on an
unintialized rv. After setting it to NS_ERROR_FAILURE, is there is still a
chance that the second 'if' evaluates to false and returns rv without calling
any other function?

>could you just remove this?
>-  char * htmlLine2 = NULL;
>+//  char * htmlLine2 = NULL;

I thought of that but comments in the code made me leave that in. See the
comments at
<http://lxr.mozilla.org/seamonkey/source/mailnews/mime/cthandlers/vcard/mimevcrd.cpp#1018>.
It seems htmlLine2 will be used when the functionality is enabled, so I left it in.

>oh, and one last thing - if you're going to change the PR_Asserts, could you
>change them to NS_ASSERTION? PR_Asserts abort execution in debug builds, which
>is a royal pain.

What do I pass as the second argument? I need a string.

Comment 9

15 years ago
>After setting it to NS_ERROR_FAILURE, is there is still a
>chance that the second 'if' evaluates to false and returns rv without calling
>any other function?
It shouldn't happen, because those routines should all fail if they return null
pointers, but you can leave out the comment if you want and just use
NS_ERROR_FAILURE. I don't like NS_ERROR_FAILURE because it's not very
informative, but never mind.

you can remove that htmlline2 var completely, as I suggested. That var should
have been defined where it was used anyway, so if we ever turn on that code
again, the var will be defined where it's used.

the string for NS_ASSERTION is supposed to describe the error, so something like
"InlineTextRich object already initialized" or whatever the appropriate object
is. Or, just "mime object already initialized".
(Reporter)

Comment 10

15 years ago
Created attachment 106253 [details] [diff] [review]
patch, v3
Attachment #106085 - Attachment is obsolete: true

Comment 11

15 years ago
Comment on attachment 106253 [details] [diff] [review]
patch, v3

thx a lot, sr=bienvenu
Attachment #106253 - Flags: superreview+
(Reporter)

Comment 12

15 years ago
Created attachment 106396 [details] [diff] [review]
patch, v4

timeless (on irc) suggested I make the following changes
1. add an NS_ASERTION() after the call to NS_NewISupportsArray instead of
ignoring the error. Also suggested I file a bug (See bug 180312)
2. change a PR_FREEIF() to PR_Free() since it doesn't have some redundant code.
Attachment #106253 - Attachment is obsolete: true
(Reporter)

Updated

15 years ago
Attachment #106396 - Flags: superreview?(bienvenu)

Updated

15 years ago
Attachment #106396 - Flags: superreview?(bienvenu) → superreview+
(Reporter)

Comment 13

15 years ago
Comment on attachment 106396 [details] [diff] [review]
patch, v4

Ok, can someone give an r= (ducarroz?) so this can get checked in. Already I'm
getting a conflict in my tree because of bienvenu's checkin for bug 129631.
Don't want this patch to get stale.

Should I create a new patch that applies cleanly?
Attachment #106396 - Flags: review?(ducarroz)
QA Contact: putterman → stephend
(Reporter)

Comment 14

15 years ago
Created attachment 107149 [details] [diff] [review]
patch, v5

Gets rid of conflicts that happened due to another checkin.
Attachment #106396 - Attachment is obsolete: true
(Reporter)

Updated

15 years ago
Attachment #107149 - Flags: superreview?(bienvenu)
Attachment #107149 - Flags: review?(ducarroz)

Comment 15

15 years ago
Comment on attachment 107149 [details] [diff] [review]
patch, v5

sr=bienvenu
Attachment #107149 - Flags: superreview?(bienvenu) → superreview+
(Assignee)

Updated

15 years ago
Hardware: PC → All
(Assignee)

Comment 16

15 years ago
Comment on attachment 107149 [details] [diff] [review]
patch, v5


>Index: mime/emitters/src/nsMimeHtmlEmitter.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/mime/emitters/src/nsMimeHtmlEmitter.cpp,v
>retrieving revision 1.65
>diff -u -w -r1.65 nsMimeHtmlEmitter.cpp
>--- mime/emitters/src/nsMimeHtmlEmitter.cpp	27 Sep 2002 09:32:06 -0000	1.65
>+++ mime/emitters/src/nsMimeHtmlEmitter.cpp	22 Nov 2002 14:51:46 -0000
>@@ -207,7 +207,7 @@
>       headerNames[numHeadersAdded] = headerInfo->name;
> 
>       if (nsCRT::strcasecmp("Date", headerInfo->name) == 0)
>-        nsresult rv = GenerateDateString(headerInfo->value, &headerValues[numHeadersAdded]);
>+        (void) GenerateDateString(headerInfo->value, &headerValues[numHeadersAdded]);
>       else
>       {
>         // optimization: if we aren't in view all header view mode, we only show a small set of the total # of headers.

If this function fails, we're likely to segfault down the road a bit.  This
needs a real error-check.

Otherwise, looks good.
Attachment #107149 - Flags: review?(ducarroz) → review-
(Reporter)

Comment 17

15 years ago
Created attachment 107423 [details] [diff] [review]
patch, v6

I filed bug 181931 to address failure handling of GeneratedateString(). dmose
suggested I get rid of that part of the patch. Here's the new patch.
Attachment #107149 - Attachment is obsolete: true
(Reporter)

Comment 18

15 years ago
Comment on attachment 107423 [details] [diff] [review]
patch, v6

dmose, I've updated the patch. Can you review and if you like it, check it in
the repository?
Attachment #107423 - Flags: review?(dmose)
(Assignee)

Comment 19

15 years ago
Comment on attachment 107423 [details] [diff] [review]
patch, v6

r=dmose, carrying forward sr=bienvenu
Attachment #107423 - Flags: superreview+
Attachment #107423 - Flags: review?(dmose)
Attachment #107423 - Flags: review+
(Assignee)

Comment 20

15 years ago
Taking, since mscott's out on sabbatical.
Assignee: mscott → dmose
Target Milestone: --- → mozilla1.3alpha
(Assignee)

Updated

15 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 21

15 years ago
Created attachment 107496 [details] [diff] [review]
patch, v7

Sigh.. got another conflict in my tree due to daniel bratell's checkin for bug
181545. here's the updated patch.
(Assignee)

Comment 22

15 years ago
Patch checked in; thanks Pratik!
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 23

15 years ago
Hm, this bug looks very similar to bug 59673 (which has a somewhat similar patch
attached to it)...
Blocks: 59673
(Reporter)

Comment 24

15 years ago
Aleksey,

I didn't realise my patch conflicted with yours. Sorry about that. I think there
should be a tracking meta-bug to "Remove all warnings in Mozilla compilation"
and have it dependent on other smaller bugs. I want to get rid of many more
warnings in Mozilla (current count is at 1176) and having such a tracker bug
would help. 

What do you guys think is the best way to approach this?

Comment 25

15 years ago
I agree, a general tracking bug could be a good idea - there is such a trackiong
bug for some of the warnings (bug 59652), but not for all of them.

Updated

15 years ago
Attachment #106396 - Flags: review?(ducarroz)
Attachment #106396 - Flags: superreview+
So, I realize the patches have been landed, and I think we still have
outstanding, current bugs to cover additional warnings.

In attempting to verify this, I can't tell which 'unused variable' warnings the
patch was actually addressing (IE, they're listed on tbox by developer, if I'm
not mistaken).

Can someone verify this or at least point me to the right criteria for 'reduce'?
 Thanks.

Comment 27

15 years ago
Well, you can verify that the total number of warnings (as reported say by brad
TBox) went down as a result of this patch being applied. IMHO this qualifies as
"reduce". ;-)

OTOH the total count have also regressed somehwat since the patch was applied...

On a related note - very often the "unusedd" warnings on brad come from code like:

nsresult rv = ...;
NS_ASSERT(NS_SUCCESS(rv), ...);

and since brad is doing optimized builds, it complains that rv is unused...

Would it be a good idea to define a macro like

#ifdef DEBUG
#define DEBUG_ONLY(x) x
#else
#define DEBUG_ONLY(x)
#endif

and turn the code above into

DEBUG_ONLY(nsresult rv =) ...;
NS_ASSERT(NS_SUCCESS(rv), ...);

Does that make sense? Should I file an RFE on that? 
Clicking on the Warn: XXXX figure returns this: (note that the link will expire
in probably a day or so):
http://tinderbox.mozilla.org/SeaMonkey/warn1040254380.13333.html

And, I'm not finding a way to query past warning counts (since this was actually
checked in on 11/26/2002.

I'm going to verify by lxr, and the fact that this got reviews/sr.

If anybody objects, reopen (but actually a new bug would be better).
Status: RESOLVED → VERIFIED
(Reporter)

Comment 29

15 years ago
stephend,

The patch addressed all unused variable warnings in the mozilla/mailnews
directory code. Initially I wanted this bug to encompass all 1200 odd warnings
in Mozilla and thought I'd post a whole bunch of incremental patches. That goal
was too lofty. I think I am going to create a tracking meta bug for all warnings
and then file smaller bugs to reduce warnings in attributed to inidividual
developers. (or maybe file them on a per component basis).

Aleksey has a good point though. There are a whole bunch of warnings that crop
up because the varible is used in NS_ASSERTIONs and nowhere else. I'm okay with
Aleksey suggestion in comment 27. Another idea would be to add the following

nsresult rv =....
(void rv); NS_ASSERTION();

This gets rid of the compiler warning and generates the same code as before.

Which way should we use? I'll file another bug to fix such warnings but I want
to get a consensus on the approach to use.
(Reporter)

Updated

15 years ago
Attachment #107496 - Attachment is obsolete: true
i'm just QA, with JS knowledge, so the C\C++ stuff I can't answer, sorry.

Comment 31

15 years ago
FYI: 
I've filed bug 187528 for tracking all the compilation warnings
I've filed bug 187530 for a canonical way to deal with variables that are only
used in debug builds.
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.