warning C4244: 'argument' : conversion from 'const PRInt32' to 'float', possible loss of data in nsPresContext.h

RESOLVED FIXED in mozilla2.0b5

Status

()

Core
Graphics
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: RyanVM, Assigned: RyanVM)

Tracking

Trunk
mozilla2.0b5
x86
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [build_warning])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 years ago
The DPI rewrite caused a new warning to be spammed all over when building with MSVC.
http://hg.mozilla.org/mozilla-central/rev/585a75516573

nsPresContext.h(628) : warning C4244: 'argument' : conversion from 'const PRInt32' to 'float', possible loss of data
nsPresContext.h(629) : warning C4244: 'argument' : conversion from 'const PRInt32' to 'float', possible loss of data
nsPresContext.h(630) : warning C4244: 'argument' : conversion from 'const PRInt32' to 'float', possible loss of data
nsPresContext.h(631) : warning C4244: 'argument' : conversion from 'const PRInt32' to 'float', possible loss of data

roc, is it safe to just static cast these?
(Assignee)

Comment 2

8 years ago
Created attachment 467634 [details] [diff] [review]
Cast to floats

Seems to work.
Assignee: nobody → ryanvm
Status: NEW → ASSIGNED
Attachment #467634 - Flags: review?(roc)
(Assignee)

Updated

8 years ago
Whiteboard: [build_warning]
Comment on attachment 467634 [details] [diff] [review]
Cast to floats

You can actually write these as constructor casts: float(marginInTwips.left) etc
Attachment #467634 - Flags: review?(roc) → review+
(Assignee)

Comment 4

8 years ago
Created attachment 467643 [details] [diff] [review]
Use constructor cast instead of static cast

This patch cuts down on MSVC warning spam in every file that includes nsPresContext.h, which is a very large number.
Attachment #467634 - Attachment is obsolete: true
Attachment #467643 - Flags: review+
Attachment #467643 - Flags: approval2.0?
Attachment #467643 - Flags: approval2.0? → approval2.0+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/2d6ead22831c
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 467643 [details] [diff] [review]
Use constructor cast instead of static cast

Sorry, short leash on approvals mean that if they're backed out they can't get back in.
Attachment #467643 - Flags: approval2.0+
(In reply to comment #6)
> This appears to be causing orange on mochitest, so I backed it out.
> 
> http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1282316894.1282317410.13910.gz

(In reply to comment #7)
> Comment on attachment 467643 [details] [diff] [review]
> Use constructor cast instead of static cast
> 
> Sorry, short leash on approvals mean that if they're backed out they can't get
> back in.

If this really did create a behavioral difference perhaps the build warning here is substantial, and this bug should be investigated more. Otherwise, this bug is pretty much NPOTB and shouldn't change a thing (besides less compiler spam).
So it appears that this was not the cause of the orange, but rather probably a machine issue. It did seem like a long-shot that this bug was at fault, but it failed at least two builds in a row in exactly the same way.

I'll restore the approved flag so feel free to reland when you can. (I might even be able to take it as a ride-along patch myself)

Comment 10

8 years ago
http://hg.mozilla.org/mozilla-central/rev/5ac01a75e650
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.