Fix unsigned/signed warnings in nsGeolocation.cpp

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [build_warning])

Attachments

(1 attachment, 1 obsolete attachment)

I get these warnings when building nsGeolocation.cpp:
../../../../mozilla/dom/src/geolocation/nsGeolocation.cpp: In member function 'virtual nsresult nsGeolocationRequest::Allow()':
../../../../mozilla/dom/src/geolocation/nsGeolocation.cpp:276: warning: comparison between signed and unsigned integer expressions
../../../../mozilla/dom/src/geolocation/nsGeolocation.cpp: In member function 'virtual nsresult nsGeolocation::ClearWatch(PRInt32)':
../../../../mozilla/dom/src/geolocation/nsGeolocation.cpp:925: warning: comparison between signed and unsigned integer expressions

Filing this bug on fixing 'em.
Created attachment 436758 [details] [diff] [review]
fix

Here's a fix.

The first chunk is for a comparison between a PRTime (signed 64-bit val) and a DOMTimeStamp (unsigned 64-bit val).  Patch casts the PRTime to be a DOMTimeStamp, so they're both unsigned. (This is what already happens under-the-hood anyway)

The second chunk is a comparison between PRUint32 and PRInt32, and when we hit it, we've already checked that the PRInt32 is >=0. (earlier in the "if" statement).  So it's safe to cast that value to a PRUint32 for the later comparison.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #436758 - Flags: review?(dougt)
Not completely sure about the first chunk -- the "- maximumAge" might bring us below 0, which would make our cast-to-unsigned be a bit bogus.

Perhaps it'd be better to create a PRTime out of cachedPositionTime, so we'd be doing our comparison in (signed) PRTime space, instead of in (unsigned) DOMTimeStamp space?  This would almost certainly be safe -- it could only fail if cachedPositionTime were ridiculously large.
Created attachment 436759 [details] [diff] [review]
fix v2

Seems safer to have the first chunk do its comparison in a signed number-space, given the subtraction involved. So, I changed the cast as described in comment 2.
Attachment #436758 - Attachment is obsolete: true
Attachment #436759 - Flags: review?(dougt)
Attachment #436758 - Flags: review?(dougt)

Comment 4

9 years ago
is a "unsigned long long" always a PRTime?
No -- PRTime is the one that's _signed_.  It's typedef'd to PRInt64.
http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prtime.h#80

It comes into play here due to the use of PR_Now() (which returns PRTime).

Comment 6

9 years ago
Comment on attachment 436759 [details] [diff] [review]
fix v2

thanks!
Attachment #436759 - Flags: review?(dougt) → review+

Comment 7

9 years ago
Marking this as blocking the e10s geolocation work, as there's a lot of code being moved around, and I'd prefer not to lose these changes when merging later.
Blocks: 552822
Landed on mozilla-central: http://hg.mozilla.org/mozilla-central/rev/7c568c6d18f6
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Blocks: 562457
Whiteboard: [build_warning]
You need to log in before you can comment on or make changes to this bug.