Compile warnings cleanup

RESOLVED FIXED in Thunderbird 13.0

Status

Thunderbird
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Irving, Assigned: Irving)

Tracking

Trunk
Thunderbird 13.0
x86
Mac OS X

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Created attachment 601290 [details] [diff] [review]
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
Attachment #601290 - Flags: review?(dbienvenu)
Created attachment 601293 [details] [diff] [review]
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.
Attachment #601293 - Flags: review?(dbienvenu)

Comment 2

6 years ago
Irving, thx for working on this. Did you do a try server build for this patch?
Try build at http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=6717be760bd3

Comment 4

6 years ago
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.
Attachment #601290 - Flags: review?(dbienvenu) → review+

Comment 5

6 years ago
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.
Attachment #601293 - Flags: review?(dbienvenu) → review+

Comment 6

6 years ago
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.
Status: NEW → ASSIGNED
Created attachment 603585 [details] [diff] [review]
Compile warnings part 1, with bienvenu's nits picked.
Attachment #601290 - Attachment is obsolete: true
Attachment #603585 - Flags: review+
Created attachment 603587 [details] [diff] [review]
Warnings part 2 - SendData() with bienvenu's nits picked
Attachment #603587 - Flags: review+
Attachment #601293 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: r=bienvenu
http://hg.mozilla.org/comm-central/rev/0683a22dac2b
http://hg.mozilla.org/comm-central/rev/946cf51e8327
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: r=bienvenu
Target Milestone: --- → Thunderbird 13.0
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

6 years ago
(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.
(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

6 years ago
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.
Depends on: 716278
You need to log in before you can comment on or make changes to this bug.