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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Ed Morley [:emorley]]

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

Comment 1

[Ed Morley [:emorley]]

Attachment: Patch v1

Replies upon the double overload being added in bug 670338.

Thanks :-)

Comment 2

[Ed Morley [:emorley]]

s/Replies/Relies/

Comment 3

[Joe Drew (not getting mail)]

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.)

Comment 4

[Ed Morley [:emorley]]

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?

Comment 5

[Ed Morley [:emorley]]

Attachment: Patch v2

Presuming the answer to comment 5 is yes, this updated patch stores mP2A as PRInt32 rather than a double, silencing the build warning.

Comment 6

[Ed Morley [:emorley]]

http://dev.philringnalda.com/tbpl/?tree=Try&rev=145f59772117

Comment 7

[Ed Morley [:emorley]]

(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 (==)

:-/

Comment 8

[Joe Drew (not getting mail)]

Comment on attachment 546317 [details] [diff] [review]
Patch v2

Given try results, clearly there's a reason!

Comment 9

[Ed Morley [:emorley]]

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! :-)

Comment 10

[Ed Morley [:emorley]]

Joe, any ideas about comment 9? Thanks :-)

Comment 11

[Joe Drew (not getting mail)]

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!

Comment 12

[Ed Morley [:emorley]]

Attachment: Patch for checkin

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).

Comment 13

[Marco Bonardo [:mak]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/58b7de3f9b79

Comment 14

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/58b7de3f9b79