Last Comment Bug 670461 - Fix conversion from 'double' to 'PRUint32' build warning in gfx/src/nsRenderingContext.h(73)
: Fix conversion from 'double' to 'PRUint32' build warning in gfx/src/nsRenderi...
Status: RESOLVED FIXED
[build_warning][inbound]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Ed Morley [:emorley]
:
: Milan Sreckovic [:milan]
Mentors:
Depends on: 670338
Blocks: buildwarning
  Show dependency treegraph
 
Reported: 2011-07-09 12:38 PDT by Ed Morley [:emorley]
Modified: 2011-08-19 03:08 PDT (History)
3 users (show)
emorley: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (1.08 KB, patch)
2011-07-09 12:46 PDT, Ed Morley [:emorley]
joe: review+
Details | Diff | Splinter Review
Patch v2 (1.25 KB, patch)
2011-07-16 06:23 PDT, Ed Morley [:emorley]
joe: review-
Details | Diff | Splinter Review
Patch for checkin (1.08 KB, patch)
2011-08-15 15:56 PDT, Ed Morley [:emorley]
no flags Details | Diff | Splinter Review

Description Ed Morley [:emorley] 2011-07-09 12:38:29 PDT
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] 2011-07-09 12:46:35 PDT
Created attachment 545014 [details] [diff] [review]
Patch v1

Replies upon the double overload being added in bug 670338.

Thanks :-)
Comment 2 Ed Morley [:emorley] 2011-07-09 14:35:27 PDT
s/Replies/Relies/
Comment 3 Joe Drew (not getting mail) 2011-07-12 12:30:49 PDT
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] 2011-07-16 03:36:53 PDT
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] 2011-07-16 06:23:30 PDT
Created attachment 546317 [details] [diff] [review]
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] 2011-07-16 09:57:36 PDT
http://dev.philringnalda.com/tbpl/?tree=Try&rev=145f59772117
Comment 7 Ed Morley [:emorley] 2011-07-16 15:37:07 PDT
(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) 2011-07-20 14:27:39 PDT
Comment on attachment 546317 [details] [diff] [review]
Patch v2

Given try results, clearly there's a reason!
Comment 9 Ed Morley [:emorley] 2011-08-02 07:49:52 PDT
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] 2011-08-14 03:10:13 PDT
Joe, any ideas about comment 9? Thanks :-)
Comment 11 Joe Drew (not getting mail) 2011-08-15 13:21:14 PDT
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] 2011-08-15 15:56:37 PDT
Created attachment 553293 [details] [diff] [review]
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 14 Marco Bonardo [::mak] 2011-08-19 03:08:06 PDT
http://hg.mozilla.org/mozilla-central/rev/58b7de3f9b79

Note You need to log in before you can comment on or make changes to this bug.