Fix compiler warnings in Azure with MSVC

RESOLVED FIXED in mozilla18

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: bas.schouten, Assigned: drexler)

Tracking

unspecified
mozilla18
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=bas.schouten@live.nl][lang=c++])

Attachments

(1 attachment, 1 obsolete attachment)

Right now Azure causes a lot of compiler warnings. That should be addressed.
So this bug is meant to address the compiler warnings when compiling code under gfx/2d using the MSVC compiler.

The majority of compiler warnings here will be conversions with precision losses. Those in most cases should just be changed to use explicit type conversions.
Summary: Fix compiler warnings in Azure → Fix compiler warnings in Azure with MSVC
Assignee: nobody → andrew.quartey
Posted patch patch (obsolete) — Splinter Review
Explicit conversions between types and the use of #pragma to silence compiler warning C4251 for private members of mozilla::gfx::AlphaBoxBlur in Blur.h.
Attachment #663648 - Flags: review?(bas.schouten)
Comment on attachment 663648 [details] [diff] [review]
patch

Review of attachment 663648 [details] [diff] [review]:
-----------------------------------------------------------------

All in all this looks very good! I'll be very happy to see these compiler warnings go away. Some small comments.

::: gfx/2d/Blur.h
@@ +12,5 @@
>  namespace gfx {
>  
> +#ifdef _MSC_VER
> +#pragma warning( disable : 4251 )
> +#endif

I'm a little concerned about this being a header. It might at some point obscure something useful, maybe that's ok though.

::: gfx/2d/DrawTargetCairo.cpp
@@ +408,1 @@
>   

whitespace

::: gfx/2d/PathCairo.cpp
@@ +270,5 @@
>  
>    double x1, y1, x2, y2;
>  
>    cairo_path_extents(*mPathContext, &x1, &y1, &x2, &y2);
> +  Rect bounds((Float)x1, (Float)y1, (Float)(x2 - x1), (Float)(y2 - y1));

Float(x1), etc.

::: gfx/2d/PathHelpers.h
@@ +23,5 @@
>  
>    // Clockwise we always sweep from the smaller to the larger angle, ccw 
>    // it's vice versa.
>    if (!aAntiClockwise && (aEndAngle < aStartAngle)) {
> +    Float correction = (Float)ceil((aStartAngle - aEndAngle) / (2.0f * M_PI));

Float() etc.
Posted patch patchSplinter Review
Fixed nits.

> > +#ifdef _MSC_VER
> > +#pragma warning( disable : 4251 )
> > +#endif
> 
> I'm a little concerned about this being a header. It might at some point
> obscure something useful, maybe that's ok though.
> 
I share similar concerns too. From my readings, the 'proper solution' seems be use of explicit template specializations for |IntRect| and |Rect| to satisfy the compiler but my attempts at such a solution generated errors elsewhere. This requires some serious template tricks thus my leaving the #pragma for suppression for now.
Attachment #663648 - Attachment is obsolete: true
Attachment #663648 - Flags: review?(bas.schouten)
Attachment #664129 - Flags: review?(bas.schouten)
Comment on attachment 664129 [details] [diff] [review]
patch

Review of attachment 664129 [details] [diff] [review]:
-----------------------------------------------------------------

Bugzilla suggests a lot of weeeeird whitespace cruft in PathHelpers.h, I'm not so sure why, we seem to get those a lot lately. Please upload a fixed patch, looks good otherwise!
Attachment #664129 - Flags: review?(bas.schouten) → review+
Fixed whitespace nits.http://hg.mozilla.org/integration/mozilla-inbound/rev/96ef3b8bd9ed
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/96ef3b8bd9ed
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.