Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Bug 685322 added 8000+ compiler warnings to windows builds (of the form: conversion from 'const int32_t' to 'mozilla::gfx::Float')

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: gps, Unassigned)

Tracking

Trunk
All
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

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

Comment 2

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

Comment 4

6 years ago
(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
Depends on: 685322

Comment 5

6 years ago
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.
Summary: Excessive compiler warning: conversion from 'const int32_t' to 'mozilla::gfx::Float' → Bug 685322 added 8000+ compiler warnings to windows builds (of the form: conversion from 'const int32_t' to 'mozilla::gfx::Float')
https://hg.mozilla.org/mozilla-central/rev/78cd6a30e250
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 7

6 years ago
(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
(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.
You need to log in before you can comment on or make changes to this bug.