Fixing warnings in toolkit components (places and commandline)

RESOLVED FIXED in mozilla6

Status

()

Toolkit
Places
--
trivial
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: timeless, Assigned: mak)

Tracking

Trunk
mozilla6
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-places])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

9 years ago
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
(Reporter)

Comment 1

9 years ago
Created attachment 378408 [details] [diff] [review]
fix warnings
Attachment #378408 - Flags: review?(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
Attachment #378408 - Flags: review?(sdwilsh) → review+
(Reporter)

Updated

9 years ago
Attachment #378408 - Flags: superreview?(neil)

Comment 3

9 years ago
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.
Attachment #378408 - Flags: superreview?(neil) → superreview-
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [timeless: need new patch]
Version: unspecified → Trunk
Attachment #378408 - Attachment is obsolete: true
(Assignee)

Comment 4

8 years ago
i think Places part should be fine right now, i don't see warnings in Windows nor Linux.
(Assignee)

Comment 5

7 years ago
Created attachment 531023 [details] [diff] [review]
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.
Assignee: timeless → mak77
Attachment #531023 - Flags: review?(sdwilsh)
(Assignee)

Updated

7 years ago
Whiteboard: [timeless: need new patch]
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).
Attachment #531023 - Flags: review?(sdwilsh) → review-
(Assignee)

Comment 7

7 years ago
(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.
(In reply to comment #7)
> Never heard of this one, thanks, will update the patch soon.
It's newish.
(Assignee)

Comment 9

7 years ago
Created attachment 531142 [details] [diff] [review]
Some more Places warnings
Attachment #531023 - Attachment is obsolete: true
Attachment #531142 - Flags: review?(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
Attachment #531142 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 11

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

Comment 12

7 years ago
Created attachment 531220 [details] [diff] [review]
Some more Places warnings

addressed comments
Attachment #531142 - Attachment is obsolete: true
(Assignee)

Comment 13

7 years ago
http://hg.mozilla.org/projects/places/rev/f625e8ddfe6f
Whiteboard: [fixed-in-places]
(Assignee)

Comment 14

7 years ago
http://hg.mozilla.org/mozilla-central/rev/f625e8ddfe6f
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
You need to log in before you can comment on or make changes to this bug.