Closed Bug 1149320 Opened 5 years ago Closed 5 years ago

fix compile warnings in mailnews/extensions/

Categories

(MailNews Core :: Backend, defect, minor)

All
Linux
defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 40.0

People

(Reporter: aceman, Assigned: aceman)

References

Details

Attachments

(1 file, 2 obsolete files)

mailnews/extensions/mdn/src/nsMsgMdnGenerator.cpp:1133:83: warning: 'exitString' may be used uninitialized in this function [-Wmaybe-uninitialized]
     bundle->FormatStringFromName(exitString, params, 1, getter_Copies(failed_msg));

mailnews/extensions/smime/src/nsMsgComposeSecure.cpp: In member function 'virtual nsresult nsMsgComposeSecure::MimeCryptoWriteBlock(const char*, int32_t)':
mailnews/extensions/smime/src/nsMsgComposeSecure.cpp:1022:30: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
     if (NS_FAILED(rv) || n < size) {

mailnews/extensions/smime/src/nsMsgComposeSecure.cpp: In member function 'virtual nsresult nsMsgComposeSecure::MimeCryptoWriteBlock(const char*, int32_t)':
mailnews/extensions/smime/src/nsMsgComposeSecure.cpp:1022:30: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
     if (NS_FAILED(rv) || n < size) {

mailnews/extensions/fts3/src/fts3_porter.c:1118:16: warning: assignment from incompatible pointer type [enabled by default]
       *pzToken = (const char**)c->zToken;

mailnews/extensions/fts3/src/nsGlodaRankerFunction.cpp: In member function 'virtual nsresult nsGlodaRankerFunction::OnFunctionCall(mozIStorageValueArray*, nsIVariant**)':
mailnews/extensions/fts3/src/nsGlodaRankerFunction.cpp:93:24: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
   if (nVal != (1 + nCol))

mailnews/extensions/fts3/src/nsGlodaRankerFunction.cpp:118:40: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
   for (uint32_t iPhrase = 0; iPhrase < nPhrase; iPhrase++) {
                                        ^
mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp: In member function 'nsresult CorpusStore::UpdateData(nsIFile*, bool, uint32_t, uint32_t*, uint32_t*)':
mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp:2774:13: warning: 'error' may be used uninitialized in this function [-Wmaybe-uninitialized]
   if (error || NS_FAILED(rv))
             ^
mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp: In member function 'void nsBayesianFilter::classifyMessage(Tokenizer&, const char*, nsTArray<unsigned int>
mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp:1679:30: warning: 'H' may be used uninitialized in this function [-Wmaybe-uninitialized]
             runningProb = (S - H + 1.0) / 2.0;
mailnews/extensions/fts3/src/nsGlodaRankerFunction.cpp:129:36: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
     for (uint32_t iCol = 0; iCol < nCol; iCol++) {
Attached patch patch (obsolete) — Splinter Review
I am not that sure about the zToken change. But gcc 4.8 is happy now with (const char*), while it said the (const char**) is incompatible pointer type.
Attachment #8585742 - Flags: review?(neil)
Comment on attachment 8585742 [details] [diff] [review]
patch

>           double S, H;
>           int32_t chi_error;
>           S = chi2P(-2.0 * sArray[tokenIndex], 2.0 * clueCount, &chi_error);
>           if (!chi_error)
>             H = chi2P(-2.0 * hArray[tokenIndex], 2.0 * clueCount, &chi_error);
>+          else
>+            H = 0;
>           clueCount += 1.0;
>           double runningProb;
>           if (!chi_error)
>             runningProb = (S - H + 1.0) / 2.0;
>           else
>             runningProb = 0.5;
Stupid compiler :-( Even I can see that H isn't used if chi_error is false.

>-  bool error;
>+  bool error = false;
The compiler is technically right here, but I wonder whether it still complains if you exchange the error and the NS_FAILED(rv) in the condition ;-)

>+  while (NS_SUCCEEDED(rv)) // break on error or done
(I would probably want to rewrite this; switch the order of OpenANSIFileDesc and GetFileSize so you can safely NS_ENSURE_SUCCESS after the latter, then use a do { } while (0); loop instead; this should shut the compiler up.)

>-      *pzToken = (const char**)c->zToken;
>+      *pzToken = (const char*)c->zToken;
Yes, this makes sense. The original coder probably wanted to change pzToken from const unsigned char** to const char** and the compiler complained so he put in this cast which you have now corrected.

>   uint32_t lenMatchInfo;
>-  int32_t *aMatchinfo = (int32_t *)aArguments->AsSharedBlob(0, &lenMatchInfo);
>-  
>+  int32_t *aMatchinfo = (int32_t*)aArguments->AsSharedBlob(0, &lenMatchInfo);
>+
>   int32_t nPhrase = aMatchinfo[0];
>   int32_t nCol = aMatchinfo[1];
>-  if (nVal != (1 + nCol))
>+  if ((int32_t)nVal != (1 + nCol))
>     return NS_ERROR_INVALID_ARG;
I checked and the fts3 matchinfo values are actually unsigned, not int32_t.

>       default:
>         if (aExitCode != NS_ERROR_ABORT && !NS_IS_MSG_ERROR(aExitCode))
>           exitString = MOZ_UTF16("smtpSendFailedUnknownReason");
>       break;
>     }
> 
>+    if (!exitString)
>+      return NS_OK;
This isn't right... I'll have to needinfo Suyash(sp?) on this one.

>+  if (NS_FAILED(rv) || n < (uint32_t)size)
size should be unsigned, but that's too big a change for this bug.
Attachment #8585742 - Flags: review?(neil)
Comment on attachment 8585742 [details] [diff] [review]
patch

Suyash, I hope you recognise this code!

>     switch (aExitCode)
>     {
>       case NS_ERROR_UNKNOWN_HOST:
>       case NS_ERROR_UNKNOWN_PROXY_HOST:
>         exitString = MOZ_UTF16("smtpSendFailedUnknownServer");
>         break;
>       case NS_ERROR_CONNECTION_REFUSED:
>@@ -1099,16 +1099,19 @@ NS_IMETHODIMP nsMsgMdnGenerator::OnStopR
>         exitString = MOZ_UTF16("smtpPasswordUndefined");
>         break;
>       default:
>         if (aExitCode != NS_ERROR_ABORT && !NS_IS_MSG_ERROR(aExitCode))
>           exitString = MOZ_UTF16("smtpSendFailedUnknownReason");
>       break;
>     }
As the compiler points out, the string is not set for NS_ERROR_ABORT or any NS_IS_MSG_ERROR error. Do we already have strings for those errors? Is there a function somewhere we can call to get the string name of those errors?
Flags: needinfo?(syshagarwal)
(In reply to neil@parkwaycc.co.uk from comment #3)
> Comment on attachment 8585742 [details] [diff] [review]
> patch
> 
> Suyash, I hope you recognise this code!
>
> >     switch (aExitCode)
> >     {
> >       case NS_ERROR_UNKNOWN_HOST:
> >       case NS_ERROR_UNKNOWN_PROXY_HOST:
> >         exitString = MOZ_UTF16("smtpSendFailedUnknownServer");
> >         break;
> >       case NS_ERROR_CONNECTION_REFUSED:
> >@@ -1099,16 +1099,19 @@ NS_IMETHODIMP nsMsgMdnGenerator::OnStopR
> >         exitString = MOZ_UTF16("smtpPasswordUndefined");
> >         break;
> >       default:
> >         if (aExitCode != NS_ERROR_ABORT && !NS_IS_MSG_ERROR(aExitCode))
> >           exitString = MOZ_UTF16("smtpSendFailedUnknownReason");
> >       break;
> >     }
> As the compiler points out, the string is not set for NS_ERROR_ABORT or any
> NS_IS_MSG_ERROR error. Do we already have strings for those errors? Is there
> a function somewhere we can call to get the string name of those errors?

I couldn't find any strings for them. As they are defined in toolkit,
http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/base/ErrorList.h#20
I haven't touched them so far as part of mailnews error codes coverage. Also,
I didn't find any helper methods for the same in toolkit.
Flags: needinfo?(syshagarwal)
So what do we do with those error codes? I changed it that if we have no exitString the function just exits.

Is there any history on why those codes are excluded?
Flags: needinfo?(neil)
(In reply to aceman from comment #5)
> So what do we do with those error codes? I changed it that if we have no
> exitString the function just exits.

Aha, I found it.

It's called errorStringNameForErrorCode.

Sounds familiar to anyone?
Flags: needinfo?(neil)
(In reply to neil@parkwaycc.co.uk from comment #6)
> (In reply to aceman from comment #5)
> Aha, I found it.
> 
> It's called errorStringNameForErrorCode.
> 
> Sounds familiar to anyone?

Yeah, you seem to be right. Before bug 952493 the aExitCode was preserved in the "default" case and passed on to FormatStringFromID. After the bug, the exitString is left empty if the aExitCode is ABORT or one of the MSG errors. That is a regression. I'll file it separately for ease of tracking as it is needed for TB38.
Blocks: 1151181
No longer blocks: 1151181
Attached patch patch v2 (obsolete) — Splinter Review
Ok, the error strings part was removed from this patch. I think that leaves nothing open here. The "unused" H will still be left reported until the compiler improves.
Attachment #8585742 - Attachment is obsolete: true
Attachment #8588374 - Flags: review?(neil)
Comment on attachment 8588374 [details] [diff] [review]
patch v2

>   int32_t *aMatchinfo = (int32_t *)aArguments->AsSharedBlob(0, &lenMatchInfo);
>-  
>-  int32_t nPhrase = aMatchinfo[0];
>-  int32_t nCol = aMatchinfo[1];
>+
>+  uint32_t nPhrase = aMatchinfo[0];
>+  uint32_t nCol = aMatchinfo[1];
Might as well change the type of aMatchinfo. (And possibly the name too!)
Attachment #8588374 - Flags: review?(neil) → review+
Attached patch patch v2.1Splinter Review
Attachment #8588374 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/2681f4e66f30
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 40.0
Duplicate of this bug: 507798
You need to log in before you can comment on or make changes to this bug.