851899 - fix some compiler warnings in mailnews/imap/src/nsImap*.cpp

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[:aceman]

I get these warning with GCC 4.7. If any of them are not valid, or the fix is not wished due to other compilers, please comment at review.

mailnews/imap/src/nsImapProtocol.cpp: In member function 'virtual void nsImapProtocol::HandleMessageDownLoadLine(const char*, bool, char*)':
mailnews/imap/src/nsImapProtocol.cpp:3776:65: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
mailnews/imap/src/nsImapProtocol.cpp: In member function 'virtual void nsImapProtocol::NormalMessageEndDownload()':
mailnews/imap/src/nsImapProtocol.cpp:3955:78: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
mailnews/imap/src/nsImapProtocol.cpp: In member function 'void nsImapProtocol::HandleCurrentUrlError()':
mailnews/imap/src/nsImapProtocol.cpp:5326:12: warning: variable 'res' set but not used [-Wunused-but-set-variable]
mailnews/imap/src/nsImapProtocol.cpp: In member function 'void nsImapProtocol::OnStatusForFolder(const char*)':
mailnews/imap/src/nsImapProtocol.cpp:6555:73: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
mailnews/imap/src/nsImapProtocol.cpp: In member function 'void nsImapProtocol::Copy(const char*, const char*, bool)':
mailnews/imap/src/nsImapProtocol.cpp:7790:15: warning: variable 'formatString' set but not used [-Wunused-but-set-variable]

mailnews/imap/src/nsImapMailFolder.h: In constructor 'nsImapMailFolder::nsImapMailFolder()':
mailnews/imap/src/nsImapMailFolder.h:514:12: warning: 'nsImapMailFolder::m_folderQuotaMaxKB' will be initialized after [-Wreorder]
mailnews/imap/src/nsImapMailFolder.h:495:8: warning:   'bool nsImapMailFolder::m_compactingOfflineStore' [-Wreorder]
mailnews/imap/src/nsImapMailFolder.cpp:177:1: warning:   when initialized here [-Wreorder]
mailnews/imap/src/nsImapMailFolder.cpp: In member function 'virtual nsresult nsImapMailFolder::UpdateFolderWithListener(nsIMsgWindow*, nsIUrlListener*)':
mailnews/imap/src/nsImapMailFolder.cpp:826:7: warning: case value '2153054228' not in enumerated type 'nsresult {aka tag_nsresult}' [-Wswitch]
mailnews/imap/src/nsImapMailFolder.cpp: In constructor 'AdoptUTF8StringEnumerator::AdoptUTF8StringEnumerator(nsTArray<nsCString>*)':
mailnews/imap/src/nsImapMailFolder.cpp:6364:12: warning: 'AdoptUTF8StringEnumerator::mIndex' will be initialized after [-Wreorder]
mailnews/imap/src/nsImapMailFolder.cpp:6363:24: warning:   'nsTArray<nsCString>* AdoptUTF8StringEnumerator::mStrings' [-Wreorder]
mailnews/imap/src/nsImapMailFolder.cpp:6352:3: warning:   when initialized here [-Wreorder]
mailnews/imap/src/nsImapMailFolder.cpp: In member function 'virtual nsrefcnt AdoptUTF8StringEnumerator::Release()':
mailnews/imap/src/nsImapMailFolder.cpp:6367:2725: warning: deleting object of polymorphic class type 'AdoptUTF8StringEnumerator' which has non-virtual destructor might cause undefined behaviour [-Wdelete-non-virtual-dtor]
mailnews/imap/src/nsImapMailFolder.cpp: In member function 'virtual nsrefcnt nsImapFolderCopyState::Release()':
mailnews/imap/src/nsImapMailFolder.cpp:7706:2689: warning: deleting object of polymorphic class type 'nsImapFolderCopyState' which has non-virtual destructor might cause undefined behaviour [-Wdelete-non-virtual-dtor]

mailnews/imap/src/nsIMAPBodyShell.cpp: In member function 'virtual int32_t nsIMAPBodypart::GenerateEmptyFilling(nsIMAPBodyShell*, bool, bool)':
mailnews/imap/src/nsIMAPBodyShell.cpp:514:18: warning: the address of 'emptyString' will always evaluate as 'true' [-Waddress]

mailnews/imap/src/nsImapServerResponseParser.cpp: In member function 'virtual void nsImapServerResponseParser::PreProcessCommandToken(const char*, const char*)':
mailnews/imap/src/nsImapServerResponseParser.cpp:342:13: warning: variable 'tagToken' set but not used [-Wunused-but-set-variable]
mailnews/imap/src/nsImapServerResponseParser.cpp:343:13: warning: variable 'uidToken' set but not used [-Wunused-but-set-variable]
mailnews/imap/src/nsImapServerResponseParser.cpp: In member function 'virtual void nsImapServerResponseParser::msg_fetch()':
mailnews/imap/src/nsImapServerResponseParser.cpp:1417:57: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
mailnews/imap/src/nsImapServerResponseParser.cpp:1038:12: warning: variable 'res' set but not used [-Wunused-but-set-variable]
mailnews/imap/src/nsImapServerResponseParser.cpp: In member function 'virtual void nsImapServerResponseParser::resp_cond_state(bool)':
mailnews/imap/src/nsImapServerResponseParser.cpp:1801:37: warning: suggest parentheses around '&&' within '||' [-Wparentheses]

Comment 1

[:aceman]

Attachment: patch

Strange, the nsIMAPNamespaceList::SerializeNamespaces function seems unused and somehow not doing what the description says (the leading quote is only prepended for the first string and the PR_smprintf result is unused).

I am also not sure about the nsIMAPBodypart::GenerateEmptyFilling change I did. Is this what the comment there requested?

Comment 2

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 725889 [details] [diff] [review]
patch

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

Looks good; a couple of questions to resolve before review is complete.

::: mailnews/imap/src/nsIMAPBodyShell.cpp
@@ +521,5 @@
>    {
>      if (stream)
>      {
> +      aShell->GetConnection()->Log("SHELL", "GENERATE-Filling", m_partNumberString);
> +      aShell->GetConnection()->HandleMessageDownLoadLine(NS_ConvertUTF16toUTF8(emptyString).get(), false);

This line is long; it either should be folded, or use "<type> conn = aShell.getConnection()" and use the temp variable in both lines

::: mailnews/imap/src/nsIMAPNamespace.cpp
@@ +283,5 @@
> + * prefixes is an array of strings;  len is the length of that array.
> + * If there is only one string, simply copy it and return it.
> + * Otherwise, put them in quotes and comma-delimit them.
> + * Returns a newly allocated string.
> + */

Wording is a bit off here, since we're not returning standalone malloc() heap that the caller needs to free. How about:

If len is one, copies the first element of prefixes into serializedNamespaces; if len > 1, copies len strings from prefixes into serializedNamespaces as a comma-separated list of quoted strings.

::: mailnews/imap/src/nsImapOfflineSync.cpp
@@ +850,5 @@
>          {
>            ClearDB();
> +          // TODO: is this used for anything?
> +          // if (deletedGhostMsgs)
> +          //   deletedAllOfflineEventsInFolder = m_currentFolder;

The code that used "deletedAllOfflineEventsInFolder" appears to have been deleted in bug 704707.

my trick: "hg log -p mailnews/imap/src/nsImapOfflineSync.cpp| less" retrieves all patches applied to that file. Then within less, search for deletedAllOfflineEventsInFolder to find the patches that affect the variable or line you care about.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +6554,2 @@
>            || prevNumMessages != GetServerStateParser().NumberOfMessages())
>        m_imapMailFolderSink->OnNewIdleMessages();

Not sure this is the right parentheses here - can you check the patch history on this line? I'm guessing we want m_imapMailFolderSink && (test1 || test2) to make sure we don't call m_imapMailFolderSink->OnNewIdleMessages() on a null object pointer.

::: mailnews/imap/src/nsImapServerResponseParser.cpp
@@ +1053,2 @@
>        if (fCurrentResponseUID == 0)
> +        (void) fFlagState->GetUidOfMessage(fFetchResponseIndex - 1, &fCurrentResponseUID);

Good question; I don't know off the top of my head, but needinfo? me if you can't convince yourself and I'll take a closer look.

Comment 3

[:aceman]

(In reply to Irving Reid (:irving) from comment #2)
> ::: mailnews/imap/src/nsImapProtocol.cpp
> @@ +6554,2 @@
> >            || prevNumMessages != GetServerStateParser().NumberOfMessages())
> >        m_imapMailFolderSink->OnNewIdleMessages();
> 
> Not sure this is the right parentheses here - can you check the patch
> history on this line? I'm guessing we want m_imapMailFolderSink && (test1 ||
> test2) to make sure we don't call m_imapMailFolderSink->OnNewIdleMessages()
> on a null object pointer.
I see what you mean, but it changes the logic. Till now the && took precedence so my brackets did not change it. It may the missing brackets were a bug since the code was introduced (in a big patch so I could not infer anything).

> ::: mailnews/imap/src/nsImapServerResponseParser.cpp
> @@ +1053,2 @@
> >        if (fCurrentResponseUID == 0)
> > +        (void) fFlagState->GetUidOfMessage(fFetchResponseIndex - 1, &fCurrentResponseUID);
> 
> Good question; I don't know off the top of my head, but needinfo? me if you
> can't convince yourself and I'll take a closer look.
No other call in the file checks the result so I make this one the same (also without void).

Comment 4

[:aceman]

Attachment: patch v2

Fixed the issues.

Comment 5

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 731568 [details] [diff] [review]
patch v2

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

r+ on everything except the parentheses in nsImapProtocol::OnStatusForFolder.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +6502,5 @@
>      int32_t prevNumMessages = GetServerStateParser().NumberOfMessages();
>      Noop();
>      // OnNewIdleMessages will cause the ui thread to update the folder
> +    if (m_imapMailFolderSink && (GetServerStateParser().NumberOfRecentMessages()
> +          || prevNumMessages != GetServerStateParser().NumberOfMessages()))

Since I'm not sure about the correct logic here, I'd like to hear from David before we check this change in. You could either take this change out (and leave the warning) or hold the rest of the patch until we sort it out.

The relevant change was https://bugzilla.mozilla.org/attachment.cgi?id=150230&action=diff

Comment 6

[David :Bienvenu]

yes, the added parentheses are the correct logic. We want to make sure m_imapMailFolderSink is not null before dereferencing it.

Comment 7

[:aceman]

Thanks guys.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/174f5ed7dc28