493783 - Fixing warnings in toolkit components (places and commandline)

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[timeless]

toolkit/components/places/src/nsNavHistoryExpire.cpp: In member function ‘nsresult nsNavHistoryExpire::EraseVisits(mozIStorageConnection*, const nsTArray<nsNavHistoryExpireRecord>&)’:
toolkit/components/places/src/nsNavHistoryExpire.cpp:584: warning: comparison between signed and unsigned integer expressions
toolkit/components/places/src/nsNavHistoryExpire.cpp:591: warning: comparison between signed and unsigned integer expressions
toolkit/components/places/src/nsNavHistoryExpire.cpp: In member function ‘nsresult nsNavHistoryExpire::EraseHistory(mozIStorageConnection*, nsTArray<nsNavHistoryExpireRecord>&)’:
toolkit/components/places/src/nsNavHistoryExpire.cpp:674: warning: comparison between signed and unsigned integer expressions
toolkit/components/places/src/nsNavHistoryExpire.cpp: In member function ‘nsresult nsNavHistoryExpire::EraseFavicons(mozIStorageConnection*, const nsTArray<nsNavHistoryExpireRecord>&)’:
toolkit/components/places/src/nsNavHistoryExpire.cpp:731: warning: comparison between signed and unsigned integer expressions
toolkit/components/places/src/nsNavHistoryExpire.cpp: In member function ‘nsresult nsNavHistoryExpire::EraseAnnotations(mozIStorageConnection*, const nsTArray<nsNavHistoryExpireRecord>&)’:
toolkit/components/places/src/nsNavHistoryExpire.cpp:770: warning: comparison between signed and unsigned integer expressions
toolkit/components/places/src/nsNavBookmarks.cpp: In member function ‘nsresult nsNavBookmarks::CreateContainerWithID(PRInt64, PRInt64, const nsACString_internal&, const nsAString_internal&, PRBool, PRInt32*, PRInt64*)’:
toolkit/components/places/src/nsNavBookmarks.cpp:1352: warning: enumeral mismatch in conditional expression: ‘nsINavBookmarksService::<anonymous enum>’ vs ‘nsINavBookmarksService::<anonymous enum>’
toolkit/components/places/src/nsFaviconService.h: In constructor ‘nsFaviconService::nsFaviconService()’:
toolkit/components/places/src/nsFaviconService.h:149: warning: ‘nsFaviconService::mFailedFaviconSerial’ will be initialized after
toolkit/components/places/src/nsFaviconService.h:147: warning:   ‘bool nsFaviconService::mExpirationRunning’
toolkit/components/places/src/nsFaviconService.cpp:145: warning:   when initialized here
toolkit/components/places/src/nsFaviconService.cpp: In member function ‘virtual nsresult FaviconLoadListener::OnStopRequest(nsIRequest*, nsISupports*, nsresult)’:
toolkit/components/places/src/nsFaviconService.cpp:1227: warning: comparison is always true due to limited range of data type
toolkit/components/places/src/nsNavHistoryAutoComplete.cpp: In member function ‘nsresult nsNavHistory::PerformAutoComplete()’:
toolkit/components/places/src/nsNavHistoryAutoComplete.cpp:586: warning: suggest parentheses around assignment used as truth value

 : In member function ‘virtual nsresult nsCommandLine::GetArgument(PRInt32, nsAString_internal&)’:
toolkit/components/commandlines/src/nsCommandLine.cpp:132: warning: comparison between signed and unsigned integer expressions
toolkit/components/commandlines/src/nsCommandLine.cpp: In member function ‘virtual nsresult nsCommandLine::RemoveArguments(PRInt32, PRInt32)’:
toolkit/components/commandlines/src/nsCommandLine.cpp:168: warning: comparison between signed and unsigned integer expressions

Comment 1

[timeless]

Attachment: fix warnings

Comment 2

[Shawn Wilsher :sdwilsh]

Comment on attachment 378408 [details] [diff] [review]
fix warnings

I'm not a reviewer of the commandline stuff, but I bet nobody will care.

r=sdwilsh

Comment 3

[[e-mail]]

Comment on attachment 378408 [details] [diff] [review]
fix warnings

> nsCommandLine::GetArgument(PRInt32 aIndex, nsAString& aResult)
> {
>   NS_ENSURE_ARG_MIN(aIndex, 0);
>-  NS_ENSURE_ARG_MAX(aIndex, mArgs.Length());
>+  NS_ENSURE_ARG_MAX((PRUint32)aIndex, mArgs.Length());
Would be nicer if we could get these changed to unsigned long in the first place...

>-    if (moreChunksToSearch = mPreviousChunkOffset != -1)
>+    if (moreChunksToSearch = (mPreviousChunkOffset != -1))
This was fixed wrongly, possibly because the original code was confusingly written, so it would be help to rewrite this to be more readable.

>+    if (deletedPlaceIdsArray.IndexOf(aRecords[i].placeID) == nsTArray_base::NoIndex) {
These should use deletedPlaceIdsArray.NoIndex etc.

Comment 4

[Marco Bonardo [:mak]]

i think Places part should be fine right now, i don't see warnings in Windows nor Linux.

Comment 5

[Marco Bonardo [:mak]]

Attachment: Some more Places warnings

This fixes some more warnings found in gcc while building Places, includes some warning fix to Storage that is seen often when building Places.

Doesn't fix all of them, deprecation ones should be handled apart (we already have a bug for that iirc), and there is a warn_unused_result warning in SendNotifyVisited that comes from some automated built ipc code, would be easy to fix it on our side, but it should probably be fixed on the ipc side.

I don't see warnings in toolkit/components/commandline, so I suppose once this patch lands this bug can be closed and new ones can be filed when needed.

Comment 6

[Shawn Wilsher :sdwilsh]

Comment on attachment 531023 [details] [diff] [review]
Some more Places warnings

>+#ifdef DEBUG
>+    nsresult rv =
>+#endif
In general, it's better to #include "mozilla/Util.h", and then do:
DebugOnly<nsresult> rv = 

(on the same line as what you are storing into it if there is space).

Comment 7

[Marco Bonardo [:mak]]

(In reply to comment #6)
> In general, it's better to #include "mozilla/Util.h", and then do:
> DebugOnly<nsresult> rv = 

Never heard of this one, thanks, will update the patch soon.

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> Never heard of this one, thanks, will update the patch soon.
It's newish.

Comment 9

[Marco Bonardo [:mak]]

Attachment: Some more Places warnings

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 531142 [details] [diff] [review]
Some more Places warnings

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

r=sdwilsh with comments addressed.

::: toolkit/components/places/nsNavHistory.cpp
@@ +4012,5 @@
>  nsNavHistory::EndUpdateBatch()
>  {
>    if (--mBatchLevel == 0) {
>      if (mBatchDBTransaction) {
> +      mozilla::DebugOnly<nsresult> rv = mBatchDBTransaction->Commit();

We should have a using namespace mozilla in this file so we don't need the namespace here.

::: toolkit/components/places/nsNavHistoryQuery.cpp
@@ +450,5 @@
>                                 NS_LITERAL_CSTRING(QUERYKEY_DOMAIN_IS_HOST),
>                                 query, &nsINavHistoryQuery::GetDomainIsHost);
>        nsCAutoString domain;
>        nsresult rv = query->GetDomain(domain);
> +      NS_ENSURE_SUCCESS(rv, rv);

This change is more than just fixing a warning.  Why are you doing it?

@@ +1654,5 @@
>                           nsINavHistoryQuery* aQuery,
>                           BoolQueryGetter getter)
>  {
>    PRBool value;
> +  mozilla::DebugOnly<nsresult> rv = (aQuery->*getter)(&value);

ditto about using namespace mozilla.

::: toolkit/components/places/nsPlacesImportExportService.cpp
@@ +698,5 @@
>          return;
>        }
>  
>        PRInt64 placesRoot;
> +      mozilla::DebugOnly<nsresult> rv = mBookmarksService->GetPlacesRoot(&placesRoot);

using namespace mozilla here too please

Comment 11

[Marco Bonardo [:mak]]

(In reply to comment #10)
> ::: toolkit/components/places/nsNavHistoryQuery.cpp
> @@ +450,5 @@
> >                                 NS_LITERAL_CSTRING(QUERYKEY_DOMAIN_IS_HOST),
> >                                 query, &nsINavHistoryQuery::GetDomainIsHost);
> >        nsCAutoString domain;
> >        nsresult rv = query->GetDomain(domain);
> > +      NS_ENSURE_SUCCESS(rv, rv);
> 
> This change is more than just fixing a warning.  Why are you doing it?

There is practically no way this thing can fail (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistoryQuery.cpp#1125), if in future we should change that, just warning about it sounds crazy, since it would then build a nonsense query.

Comment 12

[Marco Bonardo [:mak]]

Attachment: Some more Places warnings

addressed comments

Comment 13

[Marco Bonardo [:mak]]

http://hg.mozilla.org/projects/places/rev/f625e8ddfe6f

Comment 14

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/f625e8ddfe6f