Closed Bug 757960 Opened 8 years ago Closed 8 years ago

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

Categories

(Toolkit :: Places, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: dholbert, Assigned: dzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
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
Assignee: nobody → dzbarsky
Attached patch PatchSplinter Review
Attachment #633935 - Flags: review?(dietrich)
Attachment #633935 - Flags: review?(dietrich) → review?(mak77)
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
Attachment #633935 - Flags: review?(mak77) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/aba424421b31
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/aba424421b31
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
FWIW: I'm still seeing a lot of these same (or similar) warnings, so I filed bug 788792.
You need to log in before you can comment on or make changes to this bug.