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.
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 on attachment 436759 [details] [diff] [review] fix v2 thanks!
Attachment #436759 - Flags: review?(dougt) → review+
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.
Landed on mozilla-central: http://hg.mozilla.org/mozilla-central/rev/7c568c6d18f6
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.