Closed Bug 179775 Opened 22 years ago Closed 22 years ago

Reduce the number of warnings while compiling Mozilla

Categories

(MailNews Core :: Backend, defect)

All
Linux
defect
Not set
trivial

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.3alpha

People

(Reporter: pratik.solanki, Assigned: dmosedale)

References

Details

Attachments

(1 file, 6 obsolete files)

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.
CCing people whose warnings this patch fixes.
QA Contact: gayatri → putterman
Keywords: patch
-  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.
bienvenu,

Thanks for your comments. I'll post an updated patch tomorrow (hopefully).
Attached patch v2 of patch (obsolete) — Splinter Review
This addresses bienvenu's comments.
Attachment #105999 - Attachment is obsolete: true
R=ducarroz for changes in mime and smime
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.
>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.
>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".
Attached patch patch, v3 (obsolete) — Splinter Review
Attachment #106085 - Attachment is obsolete: true
Comment on attachment 106253 [details] [diff] [review]
patch, v3

thx a lot, sr=bienvenu
Attachment #106253 - Flags: superreview+
Attached patch patch, v4 (obsolete) — Splinter Review
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
Attachment #106396 - Flags: superreview?(bienvenu)
Attachment #106396 - Flags: superreview?(bienvenu) → superreview+
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
Attached patch patch, v5 (obsolete) — Splinter Review
Gets rid of conflicts that happened due to another checkin.
Attachment #106396 - Attachment is obsolete: true
Attachment #107149 - Flags: superreview?(bienvenu)
Attachment #107149 - Flags: review?(ducarroz)
Comment on attachment 107149 [details] [diff] [review]
patch, v5

sr=bienvenu
Attachment #107149 - Flags: superreview?(bienvenu) → superreview+
Hardware: PC → All
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-
Attached patch patch, v6Splinter Review
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
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)
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+
Taking, since mscott's out on sabbatical.
Assignee: mscott → dmose
Target Milestone: --- → mozilla1.3alpha
Status: NEW → ASSIGNED
Attached patch patch, v7 (obsolete) — Splinter Review
Sigh.. got another conflict in my tree due to daniel bratell's checkin for bug
181545. here's the updated patch.
Patch checked in; thanks Pratik!
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Hm, this bug looks very similar to bug 59673 (which has a somewhat similar patch
attached to it)...
Blocks: 59673
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?
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.
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.
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
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.
Attachment #107496 - Attachment is obsolete: true
i'm just QA, with JS knowledge, so the C\C++ stuff I can't answer, sorry.
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.

Attachment

General

Creator:
Created:
Updated:
Size: