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

RESOLVED FIXED in mozilla9

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: emorley, Assigned: emorley)

Tracking

(Blocks: 1 bug)

Trunk
mozilla9
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [build_warning][inbound])

Attachments

(1 attachment, 2 obsolete attachments)

1.08 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
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
(Assignee)

Updated

6 years ago
Depends on: 670338
(Assignee)

Comment 1

6 years ago
Created attachment 545014 [details] [diff] [review]
Patch v1

Replies upon the double overload being added in bug 670338.

Thanks :-)
Attachment #545014 - Flags: review?(joe)
(Assignee)

Comment 2

6 years ago
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-
(Assignee)

Comment 4

6 years ago
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?
(Assignee)

Comment 5

6 years ago
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.
Attachment #545014 - Attachment is obsolete: true
Attachment #546317 - Flags: review?(joe)
(Assignee)

Comment 6

6 years ago
http://dev.philringnalda.com/tbpl/?tree=Try&rev=145f59772117
(Assignee)

Comment 7

6 years ago
(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 (==)

:-/
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 9

6 years ago
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! :-)
(Assignee)

Comment 10

6 years ago
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!
(Assignee)

Updated

6 years ago
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)
(Assignee)

Comment 12

6 years ago
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).
Attachment #545014 - Attachment is obsolete: true
Attachment #546317 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/58b7de3f9b79
Keywords: checkin-needed
Whiteboard: [build_warning] → [build_warning][inbound]
(Assignee)

Updated

6 years ago
Target Milestone: --- → mozilla9
http://hg.mozilla.org/mozilla-central/rev/58b7de3f9b79
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.