757960 - GCC 4.6.3 build warnings in nsNavHistoryResult.cpp for unsigned >= 0 (or < 0) always returning the same value [-Wtype-limits]

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Daniel Holbert [:dholbert]]

GCC 4.6.3 spams a bunch of [-Wtype-limits] warnings for nsNavHistoryResult.cpp:

../../../../mozilla/toolkit/components/places/nsNavHistoryResult.cpp: In member function 'bool nsNavHistoryContainerResultNode::DoesChildNeedResorting(PRUint32, nsNavHistoryContainerResultNode::SortComparator, const char*)':
../../../../mozilla/toolkit/components/places/nsNavHistoryResult.cpp:992:24: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
../../../../mozilla/toolkit/components/places/nsNavHistoryResult.cpp: In member function 'bool nsNavHistoryContainerResultNode::EnsureItemPosition(PRUint32)':
../../../../mozilla/toolkit/components/places/nsNavHistoryResult.cpp:1571:24: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
../../../../mozilla/toolkit/components/places/nsNavHistoryResult.cpp:1572:16: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
../../../../mozilla/toolkit/components/places/nsNavHistoryResult.cpp: In member function 'nsresult nsNavHistoryContainerResultNode::RemoveChildAt(PRInt32, bool)':
../../../../mozilla/toolkit/components/places/nsNavHistoryResult.cpp:1739:32: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
../../../../mozilla/toolkit/components/places/nsNavHistoryResult.cpp: In member function 'virtual nsresult nsNavHistoryFolderResultNode::OnItemMoved(PRInt64, PRInt64, PRInt32, PRInt64, PRInt32, PRUint16, const nsACString_internal&, const nsACString_internal&, const nsACString_internal&)':
../../../../mozilla/toolkit/components/places/nsNavHistoryResult.cpp:4134:25: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]

I believe these are all cases where we're sanity-checking a value passed in to (or returned by) an API, and the compare-to-zero sanity-check is unnecessary & can just be dropped.

Comment 1

[Daniel Holbert [:dholbert]]

The first warning is for e.g.:
988 bool
989 nsNavHistoryContainerResultNode::DoesChildNeedResorting(PRUint32 aIndex,
990     SortComparator aComparator, const char* aData)
991 {
992   NS_ASSERTION(aIndex >= 0 && aIndex < PRUint32(mChildren.Count()),
993                "Input index out of range");

The aIndex >= 0 check can just be dropped there.

https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistoryResult.cpp#988

Comment 2

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 633935 [details] [diff] [review]
Patch

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

r=me with these additional checks:

::: toolkit/components/places/nsNavHistoryResult.cpp
@@ +1462,1 @@
>      return false;

hm, most of the calls here come from previous calls to FindChild() that just does an indexOf in the children array... So something went wrong when the unsigned argument was decided here.

At this point I think this is fine but we have to check callers so they don't try to pass -1 and it gets converted blindly.

This needs an assertion and an if check (as we do in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistoryResult.cpp#1748):
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistoryResult.cpp#695

same here
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistoryResult.cpp#3890

here just the if check (there is already an assertion earlier)
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistoryResult.cpp#4028

Comment 4

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aba424421b31

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/aba424421b31

Comment 6

[Daniel Holbert [:dholbert]]

FWIW: I'm still seeing a lot of these same (or similar) warnings, so I filed bug 788792.