731262 - Compile warnings cleanup

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

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

Attachment: Compile warnings part 1

Get rid of compile-time warnings in mail and mailnews.

Part 1:

    mailnews/base/src/nsMessenger.cpp: In function ‘int CompareAttachmentPartId(const char*, const char*)’:
    mailnews/base/src/nsMessenger.cpp:2292: warning: deprecated conversion from string constant to ‘char*’
    mailnews/base/src/nsMessenger.cpp:2295: warning: deprecated conversion from string constant to ‘char*’
Added some casts to work around const-incorrect standard definition of strtol()

    mailnews/base/util/nsMsgDBFolder.cpp: In member function ‘virtual nsresult nsMsgDBFolder::RemoveKeywordsFromMessages(nsIArray*, const nsACString_internal&)’:
    mailnews/base/util/nsMsgDBFolder.cpp:5877: warning: comparison between signed and unsigned integer expressions
Cast the unsigned Length() to signed to shut up warning - safe as long as the string is never more tan 2 billion characters long...

    mailnews/base/util/nsMsgUtils.cpp: In function ‘nsresult MsgDetectCharsetFromFile(nsILocalFile*, nsACString_internal&)’:
    mailnews/base/util/nsMsgUtils.cpp:2317: warning: comparison is always false due to limited range of data type
    mailnews/base/util/nsMsgUtils.cpp:2318: warning: comparison is always false due to limited range of data type
    mailnews/base/util/nsMsgUtils.cpp:2321: warning: comparison is always false due to limited range of data type
    mailnews/base/util/nsMsgUtils.cpp:2322: warning: comparison is always false due to limited range of data type
    mailnews/base/util/nsMsgUtils.cpp:2325: warning: comparison is always false due to limited range of data type
    mailnews/base/util/nsMsgUtils.cpp:2326: warning: comparison is always false due to limited range of data type
    mailnews/base/util/nsMsgUtils.cpp:2327: warning: comparison is always false due to limited range of data type
This was a real bug; we wouldn't have detected the BOM correctly on platforms with signed char

    mailnews/base/src/nsMsgMailSession.cpp: In member function ‘nsresult nsMsgShutdownService::ProcessNextTask()’:
    mailnews/base/src/nsMsgMailSession.cpp:577: warning: comparison between signed and unsigned integer expressions
Change mTaskIndex to PRInt32 (rather than unsigned) so that it doesn't mismatch nsTArray<>::Count(), which could be unsigned but isn't.

    mailnews/base/util/nsImapMoveCoalescer.cpp: In member function ‘nsresult nsImapMoveCoalescer::AddMove(nsIMsgFolder*, nsMsgKey)’:
    mailnews/base/util/nsImapMoveCoalescer.cpp:83: warning: comparison between signed and unsigned integer expressions
Test against the defined nsTArray<>::NoIndex constant instead of -1

    mailnews/base/src/nsMsgFolderDataSource.cpp: In constructor ‘nsMsgFolderDataSource::nsMsgFolderDataSource()’:
    mailnews/base/src/nsMsgFolderDataSource.cpp:162: warning: unused variable ‘res’
Removed declaration

    mailnews/base/src/nsMsgFolderCompactor.cpp: In member function ‘virtual nsresult nsFolderCompactState::OnStopRequest(nsIRequest*, nsISupports*, nsresult)’:
    mailnews/base/src/nsMsgFolderCompactor.cpp:630: warning: comparison between signed and unsigned integer expressions
    mailnews/base/src/nsMsgFolderCompactor.cpp: In member function ‘nsresult nsOfflineStoreCompactState::CopyNextMessage(bool&)’:
    mailnews/base/src/nsMsgFolderCompactor.cpp:905: warning: comparison between signed and unsigned integer expressions
    mailnews/base/src/nsMsgFolderCompactor.cpp:950: warning: comparison between signed and unsigned integer expressions
    mailnews/base/src/nsMsgFolderCompactor.cpp: In member function ‘virtual nsresult nsFolderCompactState::EndCopy(nsISupports*, nsresult)’:
    mailnews/base/src/nsMsgFolderCompactor.cpp:1112: warning: comparison between signed and unsigned integer expressions
Change m_curIndex to unsigned and init to 0 (instead of -1) in the constructor; it was always reset to 0 by the ::Init() method and the possibility of a negative value is never checked

    mailnews/base/src/nsMsgThreadedDBView.cpp: In member function ‘void nsMsgThreadedDBView::MoveThreadAt(nsMsgViewIndex)’:
    mailnews/base/src/nsMsgThreadedDBView.cpp:780: warning: comparison between signed and unsigned integer expressions
Add a cast

    mailnews/base/src/nsMsgQuickSearchDBView.cpp: In member function ‘virtual nsresult nsMsgQuickSearchDBView::DoCommand(nsMsgViewCommandTypeValue)’:
    mailnews/base/src/nsMsgQuickSearchDBView.cpp:130: warning: comparison between signed and unsigned integer expressions
Use an unsigned temporary

    mailnews/base/src/nsMsgQuickSearchDBView.cpp: In member function ‘virtual nsresult nsMsgQuickSearchDBView::AddHdr(nsIMsgDBHdr*, nsMsgViewIndex*)’:
    mailnews/base/src/nsMsgQuickSearchDBView.cpp:163: warning: comparison between signed and unsigned integer expressions
    mailnews/base/src/nsMsgQuickSearchDBView.cpp: In member function ‘virtual nsresult nsMsgQuickSearchDBView::ListCollapsedChildren(nsMsgViewIndex, nsIMutableArray*)’:
    mailnews/base/src/nsMsgQuickSearchDBView.cpp:631: warning: comparison between signed and unsigned integer expressions
    mailnews/base/src/nsMsgQuickSearchDBView.cpp:740: warning: comparison between signed and unsigned integer expressions
    mailnews/base/src/nsMsgQuickSearchDBView.cpp: In member function ‘virtual nsresult nsMsgQuickSearchDBView::ExpansionDelta(nsMsgViewIndex, PRInt32*)’:
    mailnews/base/src/nsMsgQuickSearchDBView.cpp:821: warning: comparison between signed and unsigned integer expressions
Compare to NoIndex

    mailnews/base/src/nsMsgQuickSearchDBView.cpp: In member function ‘virtual nsresult nsMsgQuickSearchDBView::GetFirstMessageHdrToDisplayInThread(nsIMsgThread*, nsIMsgDBHdr**)’:
    mailnews/base/src/nsMsgQuickSearchDBView.cpp:483: warning: comparison between signed and unsigned integer expressions
    mailnews/base/src/nsMsgQuickSearchDBView.cpp: In member function ‘virtual nsresult nsMsgQuickSearchDBView::SortThreads(nsMsgViewSortTypeValue, nsMsgViewSortOrderValue)’:
    mailnews/base/src/nsMsgQuickSearchDBView.cpp:547: warning: comparison between signed and unsigned integer expressions
    mailnews/base/src/nsMsgQuickSearchDBView.cpp: In member function ‘virtual nsresult nsMsgQuickSearchDBView::ListIdsInThread(nsIMsgThread*, nsMsgViewIndex, PRUint32*)’:
    mailnews/base/src/nsMsgQuickSearchDBView.cpp:676: warning: comparison between signed and unsigned integer expressions
Compare to nsMsgViewIndex_None

    mailnews/base/src/nsMsgQuickSearchDBView.cpp: In member function ‘virtual nsresult nsMsgQuickSearchDBView::ListIdsInThreadOrder(nsIMsgThread*, nsMsgKey, PRInt32, PRInt32, nsMsgKey, nsMsgViewIndex*, PRUint32*)’:
    mailnews/base/src/nsMsgQuickSearchDBView.cpp:733: warning: comparison between signed and unsigned integer expressions
Change prototype to pass unsigned int instead of signed

    mailnews/base/src/nsMsgSearchDBView.cpp: In member function ‘virtual nsresult nsMsgSearchDBView::OnHdrFlagsChanged(nsIMsgDBHdr*, PRUint32, PRUint32, nsIDBChangeListener*)’:
    mailnews/base/src/nsMsgSearchDBView.cpp:339: warning: comparison between signed and unsigned integer expressions
    mailnews/base/src/nsMsgSearchDBView.cpp: In member function ‘void nsMsgSearchDBView::MoveThreadAt(nsMsgViewIndex)’:
    mailnews/base/src/nsMsgSearchDBView.cpp:603: warning: comparison between signed and unsigned integer expressions
Change definition of HdrIndex() to PRInt32 (from unsigned) because it returns -1 in some circumstances

    mailnews/base/src/nsMsgSearchDBView.cpp: In member function ‘virtual nsresult nsMsgSearchDBView::OnStopCopy(nsresult)’:
    mailnews/base/src/nsMsgSearchDBView.cpp:1086: warning: unused variable ‘numFolders’
Removed

    mailnews/base/src/nsMsgSearchDBView.cpp: In member function ‘virtual nsMsgViewIndex nsMsgSearchDBView::FindHdr(nsIMsgDBHdr*, nsMsgViewIndex, bool)’:
    mailnews/base/src/nsMsgSearchDBView.cpp:1274: warning: comparison between signed and unsigned integer expressions
    mailnews/base/src/nsMsgSearchDBView.cpp:1283: warning: comparison between signed and unsigned integer expressions
Cast

    mailnews/base/src/nsMsgXFViewThread.cpp: In member function ‘nsresult nsMsgXFViewThread::AddHdr(nsIMsgDBHdr*, bool, PRUint32&, nsIMsgDBHdr**)’:
    mailnews/base/src/nsMsgXFViewThread.cpp:208: warning: comparison between signed and unsigned integer expressions
Function prototype change

    mailnews/base/src/nsMsgGroupThread.cpp: In member function ‘virtual nsresult nsMsgGroupThread::GetChildKeyAt(PRInt32, nsMsgKey*)’:
    mailnews/base/src/nsMsgGroupThread.cpp:239: warning: comparison between signed and unsigned integer expressions
    mailnews/base/src/nsMsgGroupThread.cpp: In member function ‘virtual nsresult nsMsgGroupThread::GetChildHdrAt(PRInt32, nsIMsgDBHdr**)’:
    mailnews/base/src/nsMsgGroupThread.cpp:247: warning: comparison between signed and unsigned integer expressions
Cast

    mailnews/base/src/nsMsgGroupThread.cpp: In member function ‘virtual nsMsgViewIndex nsMsgXFGroupThread::FindMsgHdr(nsIMsgDBHdr*)’:
    mailnews/base/src/nsMsgGroupThread.cpp:869: warning: comparison between signed and unsigned integer expressions
Compare to NoIndex

    mailnews/base/src/nsMsgPrintEngine.cpp: In member function ‘virtual nsresult nsMsgPrintEngine::StartNextPrintOperation()’:
    mailnews/base/src/nsMsgPrintEngine.cpp:447: warning: comparison between signed and unsigned integer expressions
    mailnews/base/src/nsMsgPrintEngine.cpp: In member function ‘nsresult nsMsgPrintEngine::FireThatLoadOperationStartup(const nsString&)’:
    mailnews/base/src/nsMsgPrintEngine.cpp:492: warning: comparison between signed and unsigned integer expressions
Cast

    mailnews/db/msgdb/src/nsMsgDatabase.cpp: In member function ‘virtual nsresult nsMsgDBService::OpenMore(nsIMsgDatabase*, PRInt32, bool*)’:
    mailnews/db/msgdb/src/nsMsgDatabase.cpp:297: warning: comparison between signed and unsigned integer expressions
    mailnews/db/msgdb/src/nsMsgDatabase.cpp: In member function ‘void nsMsgDatabase::ClearEnumerators()’:
    mailnews/db/msgdb/src/nsMsgDatabase.cpp:553: warning: comparison between signed and unsigned integer expressions
    mailnews/db/msgdb/src/nsMsgDatabase.cpp: In member function ‘nsMsgThread* nsMsgDatabase::FindExistingThread(nsMsgKey)’:
    mailnews/db/msgdb/src/nsMsgDatabase.cpp:560: warning: comparison between signed and unsigned integer expressions
    mailnews/db/msgdb/src/nsMsgDatabase.cpp: In member function ‘void nsMsgDatabase::ClearThreads()’:
    mailnews/db/msgdb/src/nsMsgDatabase.cpp:574: warning: comparison between signed and unsigned integer expressions
    mailnews/db/msgdb/src/nsMsgDatabase.cpp: In constructor ‘nsMsgDatabase::nsMsgDatabase()’:
    mailnews/db/msgdb/src/nsMsgDatabase.cpp:1044: warning: unused variable ‘dbCount’
    mailnews/db/msgdb/src/nsMsgDatabase.cpp: In member function ‘virtual nsresult nsMsgDatabase::DeleteHeader(nsIMsgDBHdr*, nsIDBChangeListener*, bool, bool)’:
    mailnews/db/msgdb/src/nsMsgDatabase.cpp:1926: warning: comparison between signed and unsigned integer expressions
    mailnews/db/msgdb/src/nsMsgDatabase.cpp: In member function ‘virtual PRUint32 nsMsgDatabase::GetStatusFlags(nsIMsgDBHdr*, PRUint32)’:
    mailnews/db/msgdb/src/nsMsgDatabase.cpp:2043: warning: comparison between signed and unsigned integer expressions
Removed unused, cast, etc.

    mailnews/base/search/src/nsMsgBodyHandler.cpp: In member function ‘void nsMsgBodyHandler::OpenLocalFolder()’:
    mailnews/base/search/src/nsMsgBodyHandler.cpp:168: warning: unused variable ‘rv’
Test and return early if call fails

    mailnews/db/msgdb/src/nsMsgHdr.cpp: In member function ‘virtual nsresult nsMsgHdr::GetMessageOffset(PRUint64*)’:
    mailnews/db/msgdb/src/nsMsgHdr.cpp:633: warning: comparison between signed and unsigned integer expressions
Cast. I wish people wouldn't use -1 as a special value for unsigned integer values.

    mailnews/base/search/src/nsMsgSearchAdapter.cpp:1127: warning: non-local variable ‘<anonymous struct> nsMsgSearchAttribMap []’ uses anonymous type
Make it static

    mailnews/base/search/src/nsMsgSearchSession.cpp: In member function ‘virtual nsresult nsMsgSearchSession::Search(nsIMsgWindow*)’:
    mailnews/base/search/src/nsMsgSearchSession.cpp:278: warning: comparison between signed and unsigned integer expressions
    mailnews/base/search/src/nsMsgSearchSession.cpp: In member function ‘virtual nsresult nsMsgSearchSession::AddSearchHit(nsIMsgDBHdr*, nsIMsgFolder*)’:
    mailnews/base/search/src/nsMsgSearchSession.cpp:589: warning: comparison between signed and unsigned integer expressions
    mailnews/base/search/src/nsMsgSearchSession.cpp: In member function ‘nsresult nsMsgSearchSession::NotifyListenersDone(nsresult)’:
    mailnews/base/search/src/nsMsgSearchSession.cpp:608: warning: comparison between signed and unsigned integer expressions
Cast

    mailnews/compose/src/nsMsgSend.cpp: In function ‘nsresult mime_write_message_body(nsIMsgSend*, const char*, PRInt32)’:
    mailnews/compose/src/nsMsgSend.cpp:1259: warning: comparison between signed and unsigned integer expressions
Cast

    mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp: In member function ‘nsresult CorpusStore::ClearTrait(PRUint32)’:
    mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp:2879: warning: unused variable ‘tokenCount’
Removed

Comment 1

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

Attachment: Warnings part 2 - SendData()

In file included from /Users/ireid/tbird/comm-central/mailnews/imap/src/nsIMAPBodyShell.cpp:41:
../../../mozilla/dist/include/nsMsgProtocol.h:152: warning: ‘virtual PRInt32 nsMsgProtocol::SendData(nsIURI*, const char*, bool)’ was hidden
mailnews/imap/src/nsImapProtocol.h:470: warning:   by ‘nsresult nsImapProtocol::SendData(const char*, bool)’


This uncovered quite a bit of cruft. While I didn't rename the various SendData() methods in the inheritance tree, they're really not used in an OO way - each class *must* internally call its own version of SendData() in order to operate correctly.

The nsIURI* parameter is never used, so I removed it everywhere.

Return values were not handled consistently; I cleaned up all the places I noticed and were not too tricky, but there are still some code paths where return values are ignored or crudely converted between nsresult and PRInt32 types.

Comment 2

[David :Bienvenu]

Irving, thx for working on this. Did you do a try server build for this patch?

Comment 3

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

Try build at http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=6717be760bd3

Comment 4

[David :Bienvenu]

Comment on attachment 601290 [details] [diff] [review]
Compile warnings part 1

looks ok, r=me, with some nits:

please, no space before ';':

PRUint32 i = 0 ; NS_SUCCEEDED(rv) && i < GetSize() ;

when changing lines that are much longer than 80 chars, you should reformat them, e.g.,

+  nsresult SetNamespacesPrefForHost(nsIImapIncomingServer *aHost, EIMAPNamespaceType type, const char *pref);

and

+  if (!m_newSet.IsEmpty() && m_newSet[m_newSet.Length() - 1] == key || m_newSet.BinaryIndexOf(key) != m_newSet.NoIndex)

the changes to use nsMsgViewIndex_None are a bit odd:

       nsMsgViewIndex keyIndex = m_origKeys.BinaryIndexOf(msgKey);
-      if (keyIndex != kNotFound)
+      if (keyIndex != nsMsgViewIndex_None)

it works, but NoIndex seems more appropriate.

Comment 5

[David :Bienvenu]

Comment on attachment 601293 [details] [diff] [review]
Warnings part 2 - SendData()

super long line should be reformatted:

+nsresult nsMsgDBView::ListIdsInThreadOrder(nsIMsgThread *threadHdr, nsMsgKey parentKey, PRUint32 level, nsMsgViewIndex *viewIndex, PRUint32 *pNumListed)


there are a couple places where this could be NS_ENSURE_SUCCESS(rv, rv) - this should be relatively unexpected, right?

+      rv = SendData(outputBuffer.get());
+      if (NS_FAILED(rv))
+        return rv;

there's a tiny chance that removing the code that checks for a null url before sending data might cause issues - removing that code is the right thing to do, but we'll just have to be on the lookout for any fallout from that.

Comment 6

[:aceman]

This does not clash with bug 733448.
Did you pick which warnings you fix? Otherwise you would probably see the warnings I filed into bug 733448.

Comment 7

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

Attachment: Compile warnings part 1, with bienvenu's nits picked.

Comment 8

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

Attachment: Warnings part 2 - SendData() with bienvenu's nits picked

Comment 9

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/comm-central/rev/0683a22dac2b
http://hg.mozilla.org/comm-central/rev/946cf51e8327

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 603585 [details] [diff] [review]
Compile warnings part 1, with bienvenu's nits picked.

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

::: mailnews/base/search/src/nsMsgBodyHandler.cpp
@@ +166,5 @@
>  {
>    nsCOMPtr <nsIInputStream> inputStream;
>    nsresult rv = m_scope->GetInputStream(m_msgHdr, getter_AddRefs(inputStream));
> +  // Warn and return if GetInputStream fails
> +  NS_ENSURE_SUCCESS(rv, );

If this actually compile, I think it deserves a comment...

Comment 11

[David :Bienvenu]

(In reply to Serge Gautherie (:sgautherie) from comment #10)
> Comment on attachment 603585 [details] [diff] [review]
> Compile warnings part 1, with bienvenu's nits picked.
> 
> Review of attachment 603585 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/base/search/src/nsMsgBodyHandler.cpp
> @@ +166,5 @@
> >  {
> >    nsCOMPtr <nsIInputStream> inputStream;
> >    nsresult rv = m_scope->GetInputStream(m_msgHdr, getter_AddRefs(inputStream));
> > +  // Warn and return if GetInputStream fails
> > +  NS_ENSURE_SUCCESS(rv, );
> 
> If this actually compile, I think it deserves a comment...

it really doesn't deserve a comment - we do this in a lot of places, and I hate to see comments every place we do this.

Comment 12

[Serge Gautherie (:sgautherie)]

(In reply to David :Bienvenu from comment #11)
> it really doesn't deserve a comment - we do this in a lot of places, and I
> hate to see comments every place we do this.

Oh, agreed: it looked unfamiliar (compared to "if NS_FAILED(rv) return;") and surprised me ... but I understand it works for void functions.
http://mxr.mozilla.org/comm-central/search?string=NS_ENSURE_SUCCESS(rv%2C+)%3B&case=on

Comment 13

[:aceman]

It compiles but is ugly and causes warnings. See bug 716278. Serge, if you have any good idea how to shut up the warning please post in that bug.