Last Comment Bug 703028 - Bug 685322 added 8000+ compiler warnings to windows builds (of the form: conversion from 'const int32_t' to 'mozilla::gfx::Float')
: Bug 685322 added 8000+ compiler warnings to windows builds (of the form: conv...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All Windows 7
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on: 685322
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-16 11:55 PST by Gregory Szorc [:gps]
Modified: 2011-11-21 10:00 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Gregory Szorc [:gps] 2011-11-16 11:55:23 PST
On Windows builds, there are excessive warnings during builds (~8700 in one sampled build log - https://tbpl.mozilla.org/php/getParsedLog.php?id=7426355&tree=Firefox&full=1) of the form:

  Warning C4244: 'argument' conversion from 'const int32_t' to 'mozilla::gfx::Float', possible loss of data.

If we count the warnings for C4244, we find gfx is the biggest offender:

  18 e:\builds\moz2_slave\m-cen-w32\build\gfx\2d\Point.h(62) : warning C4244: 'argument' : conversion from 'const int32_t' to 'mozilla::gfx::Float', possible loss of data
  18 e:\builds\moz2_slave\m-cen-w32\build\gfx\2d\Point.h(79) : warning C4244: 'argument' : conversion from 'const int32_t' to 'mozilla::gfx::Float', possible loss of data
  36 e:\builds\moz2_slave\m-cen-w32\build\gfx\2d\Rect.h(84) : warning C4244: 'argument' : conversion from 'const int32_t' to 'mozilla::gfx::Float', possible loss of data
  42 e:\builds\moz2_slave\m-cen-w32\build\accessible\src\base\nsAccUtils.h(367) : warning C4244: '=' : conversion from 'PRUint64' to 'PRUint32', possible loss of data
  83 e:\builds\moz2_slave\m-cen-w32\build\gfx\cairo\cairo\src\cairo-wideint-private.h(194) : warning C4244: 'return' : conversion from 'cairo_int64_t' to 'int32_t', possible loss of data
 168 e:\builds\moz2_slave\m-cen-w32\build\obj-firefox\dist\include\mozilla/gfx/Rect.h(84) : warning C4244: 'argument' : conversion from 'const int32_t' to 'mozilla::gfx::Float', possible loss of data
2158 e:\builds\moz2_slave\m-cen-w32\build\obj-firefox\dist\include\mozilla\gfx\Point.h(62) : warning C4244: 'argument' : conversion from 'const int32_t' to 'mozilla::gfx::Float', possible loss of data
2158 e:\builds\moz2_slave\m-cen-w32\build\obj-firefox\dist\include\mozilla\gfx\Point.h(79) : warning C4244: 'argument' : conversion from 'const int32_t' to 'mozilla::gfx::Float', possible loss of data
4148 e:\builds\moz2_slave\m-cen-w32\build\obj-firefox\dist\include\mozilla\gfx\Rect.h(84) : warning C4244: 'argument' : conversion from 'const int32_t' to 'mozilla::gfx::Float', possible loss of data
Comment 1 Benoit Jacob [:bjacob] (mostly away) 2011-11-16 12:31:59 PST
I don't think this warning is excessive, unless the integer expression can be checked at compile time to be exactly representable as a float.
Comment 2 Gregory Szorc [:gps] 2011-11-16 13:42:27 PST
The warning occurs thousands of times when compiling. If that isn't excessive, I don't know what is.

In the ideal world, we should see no compiler warnings when building the tree and all compiler warnings would be upgraded to errors to enforce this.

I haven't looked at the code, so I can't speak for the legitimacy of the warnings. Ideally the code would be changed to make the compiler not warn. But, if that can't be done, you can always suppress the warning by either:

1) Defining a compiler flag: http://msdn.microsoft.com/en-us/library/thxezb7y.aspx
2) Using #pragma: http://msdn.microsoft.com/en-us/library/2c8f766e%28v=VS.100%29.aspx

Of those, I recommend #2, specifically with the push and pop syntax covering the fewest lines needed to suppress the warning. You don't want to suppress the warning over more code than necessary because it may silence other (more legitimate) warnings.
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2011-11-16 14:41:53 PST
(In reply to Gregory Szorc [:gps] from comment #2)
> The warning occurs thousands of times when compiling. If that isn't
> excessive, I don't know what is.

I thought that by excessive you meant that the warning was wrong. It's a very good warning, we need to fix our code.

I agree that thousands of times is too much.

> Ideally the code would be changed to make the compiler not warn.

Yes, we should do that. The way to fix this warning is to replace

  someFloat = someInt;

by

  someFloat = float(someInt);

Maybe hg annotate / hg log would tell you whom to CC on this bug.
Comment 4 Gregory Szorc [:gps] 2011-11-16 14:55:53 PST
(In reply to Benoit Jacob [:bjacob] from comment #3)
> I thought that by excessive you meant that the warning was wrong. It's a
> very good warning, we need to fix our code.

Gotta love the ambiguity of language.

Anyway, annotate points to a recent change:

changeset:   80213:677fe017e6a0
user:        Joe Drew <joe@drew.ca>
date:        Mon Nov 14 17:29:01 2011 +1300
summary:     Bug 685322 - Create explicit conversion constructors for Rect and Point taking IntRect and IntPoint. r=roc
Comment 5 Ed Morley [:emorley] 2011-11-18 18:12:53 PST
The fact we can just add 8000+ warnings without realising emphasises the need for some kind of TBPL reporting system, a la bug 187015 or even bug 703121.
Comment 6 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-21 06:58:05 PST
https://hg.mozilla.org/mozilla-central/rev/78cd6a30e250
Comment 7 Stefan 2011-11-21 07:49:26 PST
(In reply to Benoit Jacob [:bjacob] from comment #3)
> (In reply to Gregory Szorc [:gps] from comment #2)
>   someFloat = float(someInt);

IMHO this should read

   someFloat = Float (someInt);

Point.h:

     56 struct Point :
     57   public BasePoint<Float, Point> {
     58   typedef BasePoint<Float, Point> Super;
     59 
     60   Point() : Super() {}
     61   Point(Float aX, Float aY) : Super(aX, aY) {}
     62   Point(const IntPoint& point) : Super(point.x, point.y) {}
     63 };

Types.h containts

     62 
     63 typedef float Float;
     64
Comment 8 Bas Schouten (:bas.schouten) 2011-11-21 10:00:55 PST
(In reply to Stefan from comment #7)
> (In reply to Benoit Jacob [:bjacob] from comment #3)
> > (In reply to Gregory Szorc [:gps] from comment #2)
> >   someFloat = float(someInt);
> 
> IMHO this should read
> 
>    someFloat = Float (someInt);
> 
> Point.h:
> 
>      56 struct Point :
>      57   public BasePoint<Float, Point> {
>      58   typedef BasePoint<Float, Point> Super;
>      59 
>      60   Point() : Super() {}
>      61   Point(Float aX, Float aY) : Super(aX, aY) {}
>      62   Point(const IntPoint& point) : Super(point.x, point.y) {}
>      63 };
> 
> Types.h containts
> 
>      62 
>      63 typedef float Float;
>      64

You're correct.

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