nsTArray.h:442:17: warning: comparison between signed and unsigned integer expressions [-Wsign-compare], for nsNavHistory.cpp searching a uint32_t array for an int32_t

RESOLVED FIXED in mozilla21

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks 1 bug)

Trunk
mozilla21
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

7 years ago
Recently-introduced build warning:
{
In file included from /mozilla-central/toolkit/components/places/nsMaybeWeakPtr.h:11:0,
                 from /mozilla-central/toolkit/components/places/nsNavHistory.h:20,
                 from /mozilla-central/toolkit/components/places/nsNavHistory.cpp:10:
../../../dist/include/nsTArray.h: In instantiation of 'bool nsDefaultComparator<A, B>::Equals(const A&, const B&) const [with A = unsigned int; B = int]':
../../../dist/include/nsTArray.h:707:7:   required from 'nsTArray_Impl<E, Alloc>::index_type nsTArray_Impl<E, Alloc>::IndexOf(const Item&, nsTArray_Impl<E, Alloc>::index_type, const Comparator&) const [with Item'
../../../dist/include/nsTArray.h:721:71:   required from 'nsTArray_Impl<E, Alloc>::index_type nsTArray_Impl<E, Alloc>::IndexOf(const Item&, nsTArray_Impl<E, Alloc>::index_type) const [with Item = int; E = unsign'
../../../dist/include/nsTArray.h:693:29:   required from 'bool nsTArray_Impl<E, Alloc>::Contains(const Item&) const [with Item = int; E = unsigned int; Alloc = nsTArrayInfallibleAllocator]'
/mozilla-central/toolkit/components/places/nsNavHistory.cpp:1063:53:   required from here
../../../dist/include/nsTArray.h:442:17: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
}

This is from these lines:

nsNavHistory.h:
> 249 class nsNavHistoryResultNode : public nsINavHistoryResultNode
> 250 {
[...]
> 387   int32_t mTransitionType;
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistoryResult.h#387

nsNavHistory.cpp:
> 1060     const nsTArray<uint32_t>& transitions = query->Transitions();
[...]
> 1063         !transitions.Contains(aNode->mTransitionType)) {
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistory.cpp#1060

(That last line is the line pointed to by the warning message.)

As shown by the code above, aNode->mTransitionType is an int32_t, and we're passing it to "Contains" for an array that stores uint32_t.  That's bogus.  We need to either add some casts (hacky/undesirable), or make the types agree in the first place (if possible).
Assignee

Updated

7 years ago
Summary: nsTArray.h:442:17: warning: comparison between signed and unsigned integer expressions [-Wsign-compare], for nsNavHistory.cpp → nsTArray.h:442:17: warning: comparison between signed and unsigned integer expressions [-Wsign-compare], for nsNavHistory.cpp searching a uint32_t array for an int32_t
Version: unspecified → Trunk
Assignee

Comment 1

7 years ago
Looks like we can just make mTransitionType unsigned.

It's initialized to 0:
> 102   mTransitionType(0)
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistoryResult.cpp#102

...and it's only ever set in one point, to an uint32_t value:
> 2630       addition->mTransitionType = aTransitionType;
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistoryResult.cpp#2630

so AFAICT, there's no risk of it going below 0.

(NOTE: There's *another* mTransitionType variable in that file, which *is* set to a signed variable -- however, that's part of a completely-unused class that we can just drop, and I've filed bug 835543 on doing that.)
Assignee

Comment 2

7 years ago
Note also that the "transitionType" variables / parameters elsewhere in this code are all unsigned (disregarding the chunk of code that I'm removing in bug 835543).
Assignee

Comment 3

7 years ago
Posted patch fix v1Splinter Review
Attachment #707307 - Flags: review?(mak77)
Assignee

Updated

7 years ago
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Comment on attachment 707307 [details] [diff] [review]
fix v1

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

looks like all of the other problematic use-cases are taken care in the patch removing FullVisitResultNode so, provided these 2 bugs land together, will be fine.
Thanks!
Attachment #707307 - Flags: review?(mak77) → review+
Assignee

Comment 5

7 years ago
Landed bug 835543 earlier today (removing FullVisitResultNode), so this is good to land.

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/527874a5324b
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/527874a5324b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.