Closed Bug 670461 Opened 8 years ago Closed 8 years ago

Fix conversion from 'double' to 'PRUint32' build warning in gfx/src/nsRenderingContext.h(73)

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: emorley, Assigned: emorley)

References

(Blocks 1 open bug)

Details

(Whiteboard: [build_warning][inbound])

Attachments

(1 file, 2 obsolete files)

Shows up 144 times in my MSVC2010 local build...

/dist/include/nsRenderingContext.h(73) : warning C4244: 'return' : conversion from 'double' to 'PRInt32', possible loss of data

http://mxr.mozilla.org/mozilla-central/source/gfx/src/nsRenderingContext.h#73
Depends on: 670338
Attached patch Patch v1 (obsolete) — Splinter Review
Replies upon the double overload being added in bug 670338.

Thanks :-)
Attachment #545014 - Flags: review?(joe)
s/Replies/Relies/
Comment on attachment 545014 [details] [diff] [review]
Patch v1

Why is mP2A stored as a double if we only use it as a PRInt32?

(If there's a good reason, please rerequest review on this patch.)
Attachment #545014 - Flags: review?(joe) → review-
I have no idea (and had thought the same myself, but presumed I was just missing something).

So would you like me to just change it to PRInt32?
Attached patch Patch v2 (obsolete) — Splinter Review
Presuming the answer to comment 5 is yes, this updated patch stores mP2A as PRInt32 rather than a double, silencing the build warning.
Attachment #545014 - Attachment is obsolete: true
Attachment #546317 - Flags: review?(joe)
(In reply to comment #6)
> http://dev.philringnalda.com/tbpl/?tree=Try&rev=145f59772117

Failed try, by causing reftest failures on all platforms along the lines of:

REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/pixel-rounding/iframe-1.html | image comparison (==)

:-/
Flags: in-testsuite-
Comment on attachment 546317 [details] [diff] [review]
Patch v2

Given try results, clearly there's a reason!
Attachment #546317 - Flags: review?(joe) → review-
Attachment #545014 - Attachment is obsolete: false
Attachment #545014 - Flags: review- → review+
Joe, I may have just messed up the v2 patch, given there are other instances where the variable mP2A are used (not all in scope admittedly):
http://mxr.mozilla.org/mozilla-central/search?string=mP2A&case=1&tree=mozilla-central

Could you take a quick look before we resort to casting, in case changing mP2A to PRInt32 is still a better approach?

Thanks! :-)
Joe, any ideas about comment 9? Thanks :-)
Ah, yes - you're clearly right that changing the type is wrong, since it's used internally in nsRenderingContext as a double. Your first patch is therefore correct!
Summary: Fix conversion from 'double' to 'PRInt32' build warning in gfx/src/nsRenderingContext.h(73) → Fix conversion from 'double' to 'PRUint32' build warning in gfx/src/nsRenderingContext.h(73)
Great, thanks Joe :-)

Patch for checkin based on the v1 approach, but updated to tip to take account of bug 678222 landing (which changed the return type of AppUnitsPerDevPixel() to be PRUint32).
Attachment #545014 - Attachment is obsolete: true
Attachment #546317 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/58b7de3f9b79
Keywords: checkin-needed
Whiteboard: [build_warning] → [build_warning][inbound]
Target Milestone: --- → mozilla9
http://hg.mozilla.org/mozilla-central/rev/58b7de3f9b79
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.