Last Comment Bug 731262 - Compile warnings cleanup
: Compile warnings cleanup
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: General (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: Thunderbird 13.0
Assigned To: :Irving Reid (No longer working on Firefox)
:
Mentors:
Depends on: 716278
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-28 08:56 PST by :Irving Reid (No longer working on Firefox)
Modified: 2012-03-08 07:47 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Compile warnings part 1 (43.54 KB, patch)
2012-02-28 08:56 PST, :Irving Reid (No longer working on Firefox)
mozilla: review+
Details | Diff | Review
Warnings part 2 - SendData() (48.31 KB, patch)
2012-02-28 09:01 PST, :Irving Reid (No longer working on Firefox)
mozilla: review+
Details | Diff | Review
Compile warnings part 1, with bienvenu's nits picked. (43.69 KB, patch)
2012-03-06 20:21 PST, :Irving Reid (No longer working on Firefox)
irving: review+
Details | Diff | Review
Warnings part 2 - SendData() with bienvenu's nits picked (48.39 KB, patch)
2012-03-06 20:22 PST, :Irving Reid (No longer working on Firefox)
irving: review+
Details | Diff | Review

Description :Irving Reid (No longer working on Firefox) 2012-02-28 08:56:37 PST
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
Comment 1 :Irving Reid (No longer working on Firefox) 2012-02-28 09:01:23 PST
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.
Comment 2 David :Bienvenu 2012-02-28 13:00:50 PST
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) 2012-02-28 13:10:05 PST
Try build at http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=6717be760bd3
Comment 4 David :Bienvenu 2012-02-29 15:39:20 PST
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 2012-02-29 15:55:30 PST
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 2012-03-06 11:29:41 PST
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) 2012-03-06 20:21:50 PST
Created attachment 603585 [details] [diff] [review]
Compile warnings part 1, with bienvenu's nits picked.
Comment 8 :Irving Reid (No longer working on Firefox) 2012-03-06 20:22:38 PST
Created attachment 603587 [details] [diff] [review]
Warnings part 2 - SendData() with bienvenu's nits picked
Comment 10 Serge Gautherie (:sgautherie) 2012-03-07 16:53:37 PST
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 2012-03-07 16:58:05 PST
(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) 2012-03-07 17:41:13 PST
(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 2012-03-07 22:53:01 PST
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.

Note You need to log in before you can comment on or make changes to this bug.