1202150 - Silence compiler warnings in nsMsgDBView and friends

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Comment 1

[alta88]

Attachment: warnings.patch

Comment 2

[:aceman]

Comment on attachment 8657463 [details] [diff] [review]
warnings.patch

Review of attachment 8657463 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for cleaning up compiler warnings.

::: mailnews/base/src/nsMsgDBView.cpp
@@ +395,5 @@
>      }
>    }
>  
>    nsCString author;
> +  aHdr->GetAuthor(getter_Copies(author));

I see you know the (void) convention when intentionally not checking return value. So in this case you do not know whether we want to check it or not, and defer it for later?

@@ +3837,5 @@
>  
>      rv = db->CompareCollationKeys((*p1)->dword, (*p1)->key, (*p2)->dword,
>                                    (*p2)->key, &retVal);
> +    if (!NS_SUCCEEDED(rv))
> +      NS_ASSERTION(false, "compare failed");

What kind of compiler warning was produced here? Also, you could use NS_FAILED.

@@ -6504,5 @@
>  {
> -    bool threadElided = true;
> -    if (threadIndex != nsMsgViewIndex_None)
> -        threadElided = (m_flags[threadIndex] & nsMsgMessageFlags::Elided);
> -

+1, bug 1134024 comment 5.

::: mailnews/base/src/nsMsgXFVirtualFolderDBView.cpp
@@ +441,5 @@
>                {
>                  nsMsgKey msgKey;
>                  pHeader->GetMessageKey(&msgKey);
> +                if (prevKey != nsMsgKey_None && msgKey <= prevKey)
> +                  NS_ASSERTION(false, "cached Hits not sorted");

What is the warning here?

Comment 3

[alta88]

1. an oversight, later patches in this series all use void.
2. if the only reference to a var is in the NS_ASSERTION macro, it raises Wunused-variable.

Comment 4

[alta88]

Attachment: warnings2.patch

Comment 5

[Kent James (:rkent)]

Comment on attachment 8657463 [details] [diff] [review]
warnings.patch

Review of attachment 8657463 [details] [diff] [review]:
-----------------------------------------------------------------

Lots of little issues here. There are more difficult than it seems to get right, and I had to do a bit of code investigation to figure out the correct approach to all of this.

::: mailnews/base/src/nsMsgDBView.cpp
@@ +395,5 @@
>      }
>    }
>  
>    nsCString author;
> +  aHdr->GetAuthor(getter_Copies(author));

I agree, this needs (void) in front.

@@ +3835,5 @@
>  
>      nsIMsgDatabase *db = sortInfo->db;
>  
>      rv = db->CompareCollationKeys((*p1)->dword, (*p1)->key, (*p2)->dword,
>                                    (*p2)->key, &retVal);

mfbt now has a macro to solve this problem. The pattern, which is now the preferred approach, is:

mozilla::DebugOnly<nsresult> rv = db->CompareCollationKeys( ...

and then you do not have to change the NS_ASSERTION line. In some cases you may have to add #include "mozilla/DebugOnly.h" but in my tests this was not needed in nsMsgDBView.cpp (You would also remove the "nsresult rv" declaration earlier.)

@@ +3861,5 @@
>    viewSortInfo* sortInfo = (viewSortInfo *) privateData;
>  
>    nsIMsgDatabase *db = sortInfo->db;
>  
>    rv = db->CompareCollationKeys((*p1)->dword, (*p1)->key, (*p2)->dword,

As earlier, use DebugOnly here

::: mailnews/base/src/nsMsgXFVirtualFolderDBView.cpp
@@ +440,5 @@
>                if (pHeader && NS_SUCCEEDED(rv))
>                {
>                  nsMsgKey msgKey;
>                  pHeader->GetMessageKey(&msgKey);
> +                if (prevKey != nsMsgKey_None && msgKey <= prevKey)

You need to find a way to do this that does not add useless comparisons in release builds. I assume the issue is the unused prevKey variable. You could, for example, define it above using DebugOnly, and then surround the prevKey = msgKey with #ifdef(DEBUG)

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +5701,5 @@
>  nsMsgDatabase::GetCachedHits(const char *aSearchFolderUri, nsISimpleEnumerator **aEnumerator)
>  {
>    nsCOMPtr <nsIMdbTable> table;
>    nsresult err = GetSearchResultsTable(aSearchFolderUri, false, getter_AddRefs(table));
> +  NS_ENSURE_SUCCESS(err, err);

The error here is just another check for table. So we really don't want to add an NS_ENSURE_SUCCESS here as the result is expected and handled in the next line. Just use (void) instead of defining then not using err.

Comment 6

[Kent James (:rkent)]

Comment on attachment 8663916 [details] [diff] [review]
warnings2.patch

Review of attachment 8663916 [details] [diff] [review]:
-----------------------------------------------------------------

need to use DebugOnly in most of the cases here as well.

::: mailnews/addrbook/src/nsAbMDBDirectory.cpp
@@ +271,5 @@
>    if (m_AddressList)
>    {
>      uint32_t count;
>      nsresult rv;
>      rv = m_AddressList->GetLength(&count);

again, use DebugOnly

::: mailnews/base/src/nsMsgAccount.cpp
@@ +61,5 @@
>    // create the incoming server lazily
>    if (!mTriedToGetServer && !m_incomingServer) {
>      mTriedToGetServer = true;
>      // ignore the error (and return null), but it's still bad so warn
>      nsresult rv = createIncomingServer();

use DebugOnly

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +917,5 @@
>  void nsMsgAccountManager::LogoutOfServer(nsIMsgIncomingServer *aServer)
>  {
>    if (!aServer)
>      return;
>    nsresult rv = aServer->Shutdown();

Use DebugOnly here and leave the NS_ASSERTION alone.

::: mailnews/base/src/nsSubscribableServer.cpp
@@ +53,5 @@
>      nsresult rv = NS_OK;
>  #ifdef DEBUG_seth
>      printf("free subscribe tree\n");
>  #endif
>      rv = FreeSubtree(mTreeRoot);

use DebugOnly

::: mailnews/compose/src/nsMsgCompUtils.cpp
@@ +744,5 @@
>      int32_t pref_content_disposition = 0;
>  
>      if (prefs) {
>        nsresult rv = prefs->GetIntPref("mail.content_disposition_type",
>                                        &pref_content_disposition);

use DebugOnly

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +1650,5 @@
>                                   m_addresses[m_addressesLeft - 1].get());
>      }
>  
> +    if (!NS_SUCCEEDED(rv))
> +      NS_ASSERTION(false, "failed to explain SMTP error");

This one is a little trickier because of the double rv = so we can accept this change.

@@ +1701,5 @@
>    if (m_responseCode != 354)
>    {
>      nsresult rv = nsExplainErrorDetails(m_runningURL,
>                                          NS_ERROR_SENDING_DATA_COMMAND,
>                                          m_responseText.get());

use DebugOnly

@@ +1775,5 @@
>    if((m_responseCode/10 != 25))
>    {
>      nsresult rv = nsExplainErrorDetails(m_runningURL,
>                                          NS_ERROR_SENDING_MESSAGE,
>                                          m_responseText.get());

use DebugOnly

::: mailnews/db/msgdb/src/nsMsgThread.cpp
@@ +54,5 @@
>    if (m_mdbDB)
>    {
>      bool found = m_mdbDB->m_threads.RemoveElement(this);
> +    if (!found)
> +      NS_ASSERTION(false, "removing thread not in threads array");

use DebugOnly

::: mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp
@@ +1525,5 @@
>          prob =  (0.225 + n * prob) / (.45 + n);
>          double distance = std::abs(prob - 0.5);
>          if (distance >= .1)
>          {
>            nsresult rv = setAnalysis(token, traitIndex, distance, prob);

use DebugOnly

::: mailnews/extensions/mdn/src/nsMsgMdnGenerator.cpp
@@ +1026,5 @@
>      m_folder = folder;
>      m_headers = headers;
>      m_key = key;
>  
>      nsresult rv = InitAndProcess(_retval);

use DebugOnly

@@ +1052,5 @@
>   */
>  nsresult nsMsgMdnGenerator::NoteMDNRequestHandled()
>  {
>    nsresult rv = StoreMDNSentFlag(m_folder, m_key);
> +  if (!NS_SUCCEEDED(rv))

use DebugOnly

Comment 7

[alta88]

Attachment: warnings.patch

> DebugOnly

ah, nice.  although very widely used in m=c, there's just one single use in mailnews/.

Comment 8

[alta88]

Attachment: warnings2.patch

Comment 9

[alta88]

Attachment: warnings4.patch

Comment 10

[alta88]

Attachment: warnings5.patch

Comment 11

[alta88]

Attachment: warnings6.patch

Comment 12

[Kent James (:rkent)]

Comment on attachment 8667385 [details] [diff] [review]
warnings4.patch

Review of attachment 8667385 [details] [diff] [review]:
-----------------------------------------------------------------

I suppose it is not worth the effort to figure out if we should really be checking these return values on this rarely used code, so OK to just accept what we've been doing without warnings.

Comment 13

[Kent James (:rkent)]

Comment on attachment 8667386 [details] [diff] [review]
warnings5.patch

jcranmer and I had a brief IRC conversation about this issue. His reponse: " it is the NS_MSG_*-isn't-in-the-nsresult-enum issue ... the better alternative would be to stick the NS_MSG_* stuff in mozilla-central/xpcom/mumble/ErrorList.h"

I'm not prepared to make the call that we should replace these warnings with cast to int as the preferred solution. For now, I'm going to defer this decision to jcranmer.

I've long avoided creating new error types myself because of some of these issues, and just stuck with standard error types. That is not the best solution, either. Perhaps what we need is some mechanism that would allow us to extend the nsresult definition, not sure.

Comment 14

[Kent James (:rkent)]

Comment on attachment 8667386 [details] [diff] [review]
warnings5.patch

Another suggestion is that this issue is bug 783526 and we should reject the (int) cast solution.

Comment 15

[alta88]

Comment on attachment 8667386 [details] [diff] [review]
warnings5.patch

the patch is withdrawn.

Comment 16

[Kent James (:rkent)]

Comment on attachment 8667387 [details] [diff] [review]
warnings6.patch

Review of attachment 8667387 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits fixed.

::: mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp
@@ +1633,4 @@
>          for (uint32_t tokenIndex = 0; tokenIndex < usedTokenCount; tokenIndex++)
>          {
>            TraitAnalysis& ta = traitAnalyses[last - 1 - tokenIndex];
>            double S, H;

These are declared earlier in the block, so I think that the best thing to do is to just remove this declaration line.

@@ +1638,5 @@
>            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 = S;

then the else block is not needed.

::: mailnews/mime/src/mimeiimg.cpp
@@ +57,5 @@
>  static int
>  MimeInlineImage_parse_begin (MimeObject *obj)
>  {
>    MimeInlineImage *img = (MimeInlineImage *) obj;
> +  //MimeInlineImageClass *clazz;

Just delete this, not need to comment out.

@@ +72,5 @@
>        //  gunking the body up.
>        obj->options->write_pure_bodies)
>      return 0;
>  
> +  //clazz = (MimeInlineImageClass *) obj->clazz;

ditto, delete.

Comment 17

[alta88]

Attachment: warnings3.patch

Comment 18

[alta88]

Attachment: warnings4.patch

comment fix.

Comment 19

[alta88]

Attachment: warnings6.patch

udpated for comments.

Comment 20

[Kent James (:rkent)]

Comment on attachment 8667507 [details] [diff] [review]
warnings3.patch

Review of attachment 8667507 [details] [diff] [review]:
-----------------------------------------------------------------

Although there are a lot of little changes, they are all straightforward so r=me with changes done.

::: mailnews/imap/src/nsImapFlagAndUidState.cpp
@@ +41,5 @@
>  }
>  
>  NS_IMETHODIMP nsImapFlagAndUidState::SetMessageFlags(int32_t zeroBasedIndex, unsigned short flags)
>  {
> +  if (zeroBasedIndex < (signed)fUids.Length())

this should be (int32_t) not (signed)

::: mailnews/local/src/nsMsgBrkMBoxStore.cpp
@@ +225,5 @@
>    int64_t fileSize = 0;
>    uint32_t actualFolderTimeStamp = 0;
>    GetMailboxModProperties(aFolder, &fileSize, &actualFolderTimeStamp);
>  
> +  if ((signed)folderSize == fileSize && numUnreadMessages >= 0)

should be (int64_t)

::: mailnews/local/src/nsPop3Protocol.cpp
@@ +643,5 @@
>  void nsPop3Protocol::UpdateStatusWithString(const char16_t *aStatusString)
>  {
>      if (mProgressEventSink)
>      {
> +      mozilla::DebugOnly<nsresult> rv = mProgressEventSink->OnStatus(this,

This statement has now gotten way too long. Instead, put the mozilla::DebugOnly<nsresult> rv = on one line, and start mProgressEventSink->OnStatus indended 2 characters from the mozilla::...

@@ +3165,5 @@
>      }
>      else
>      {
>        nsString finalString;
> +      mozilla::DebugOnly<nsresult> rv = FormatCounterString(NS_LITERAL_STRING("receivingMessages"),

Same thing here, too long, start FormatCounterString on new line

::: mailnews/mime/cthandlers/pgpmime/nsPgpMimeProxy.cpp
@@ +94,5 @@
>    MimeObjectClass    *oclass = (MimeObjectClass *)    clazz;
>    MimeEncryptedClass *eclass = (MimeEncryptedClass *) clazz;
>  
> +  if (!oclass || !oclass->class_initialized)
> +    NS_ASSERTION(false, "oclass is not initialized");

Change the beginning of this method to:

  mozilla::DebugOnly<MimeObjectClass *> oclass = (MimeObjectClass *) clazz;
  NS_ASSERTION(!oclass->class_initialized, "oclass is not initialized");

  MimeEncryptedClass *eclass = (MimeEncryptedClass *) clazz;

::: mailnews/mime/src/mimedrft.cpp
@@ +1648,5 @@
>    int nAttachments = 0;
>    //char *hdr_value = NULL;
>    char *parm_value = NULL;
>    bool creatingMsgBody = true;
> +  //bool bodyPart = false;

All of these bodyPart lines have done nothing since first checked in, so let's just delete them.

@@ +1691,5 @@
>      if (!mdd->messageBody)
>        return MIME_OUT_OF_MEMORY;
>      newAttachment = mdd->messageBody;
>      creatingMsgBody = true;
> +    //bodyPart = true;

delete

@@ +1809,5 @@
>    nsresult rv;
>  
>    // This needs to be done so the attachment structure has a handle
>    // on the temp file for this attachment...
> +  // if ((tmpFile) && (!bodyPart))

delete

@@ +1897,5 @@
>    else
>    {
>      uint32_t bytesWritten;
>      mdd->tmpFileStream->Write(buf, size, &bytesWritten);
> +    if ((signed)bytesWritten < size)

(int32_t)

::: mailnews/news/src/nsNNTPProtocol.cpp
@@ +4135,5 @@
>    }
>  
>    if (line)
>    {
> +    mozilla::DebugOnly<nsresult> rv;

Cool, I did not realize it could be used this way, with the rv =  still working in release builds!

Comment 21

[alta88]

Attachment: warnings3.patch

updated for comments.

Comment 22

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/aac9d2367a718976c4789a93ddc4ccce5beb3f54
Bug 1202150 - Silence compiler warnings in nsMsgDBView and friends. r=rkent

https://hg.mozilla.org/comm-central/rev/4d82d455486356697812d9baa3042ee5ce1bde39
Bug 1202150 - Silence compiler warnings in nsMsgDBView and friends, part 2 mailnews/addrbook|base|compose|db|extensions. r=rkent

https://hg.mozilla.org/comm-central/rev/553773cf31bde6d107bb2baa03015b472efe1b54
Bug 1202150 - Silence compiler warnings in nsMsgDBView and friends, part 3 mailnews/imap|local|mime|news. r=rkent

https://hg.mozilla.org/comm-central/rev/e91c890a2de59d496102475d686d2d64a32b2430
Bug 1202150 - Silence compiler warnings in nsMsgDBView and friends, part 4 mail/. r=rkent

https://hg.mozilla.org/comm-central/rev/3ddf3a3eaf07d5e6c1a98bfbe184dab32841ed03
Bug 1202150 - Silence compiler warnings in nsMsgDBView and friends, part 6 final stragglers. r=rkent a=aleth,IanN SM CLOSED TREE