Closed Bug 859021 Opened 7 years ago Closed 7 years ago

Remove the MSVC exemption for FAIL_ON_WARNINGS in content/svg/content/src

Categories

(Core :: SVG, defect)

All
Windows 8
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: emk, Assigned: emk)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
No description provided.
Attachment #734324 - Flags: review?(dholbert)
For historical / clarity reasons, it looks like the warnings in question are the following (copied from a recent Try build log):

{
e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/DOMSVGPathSegList.cpp(457) : warning C4146: unary minus operator applied to unsigned type, result still unsigned

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/DOMSVGPathSegList.cpp(532) : warning C4146: unary minus operator applied to unsigned type, result still unsigned

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/SVGAElement.cpp(50) : warning C4355: 'this' : used in base member initializer list

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/SVGContentUtils.cpp(263) : warning C4305: '+=' : truncation from 'double' to 'float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/SVGContentUtils.cpp(269) : warning C4305: '+=' : truncation from 'double' to 'float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/SVGLength.cpp(101) : warning C4305: 'initializing' : truncation from 'double' to 'float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/SVGLength.cpp(101) : warning C4305: 'initializing' : truncation from 'double' to 'float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/SVGLength.cpp(101) : warning C4305: 'initializing' : truncation from 'double' to 'float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/SVGLength.cpp(101) : warning C4305: 'initializing' : truncation from 'double' to 'float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/SVGLength.cpp(103) : warning C4305: 'initializing' : truncation from 'double' to 'float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/SVGLength.cpp(103) : warning C4305: 'initializing' : truncation from 'double' to 'float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/SVGLength.cpp(103) : warning C4305: 'initializing' : truncation from 'double' to 'float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/SVGLength.cpp(105) : warning C4305: 'initializing' : truncation from 'double' to 'float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/SVGLength.cpp(105) : warning C4305: 'initializing' : truncation from 'double' to 'float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/SVGLength.cpp(105) : warning C4305: 'initializing' : truncation from 'double' to 'float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/SVGLength.cpp(105) : warning C4305: 'initializing' : truncation from 'double' to 'float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/SVGLength.cpp(107) : warning C4305: 'initializing' : truncation from 'double' to 'float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/SVGLength.cpp(107) : warning C4305: 'initializing' : truncation from 'double' to 'float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/SVGLength.cpp(109) : warning C4305: 'initializing' : truncation from 'double' to 'float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/SVGLength.cpp(109) : warning C4305: 'initializing' : truncation from 'double' to 'float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/SVGLength.cpp(109) : warning C4305: 'initializing' : truncation from 'double' to 'float'

e:\builds\moz2_slave\try-w32-d-00000000000000000000\build\config\rules.mk:1061:0$ e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/obj-firefox/_virtualenv/Scripts/

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/nsSVGFilters.cpp(722) : warning C4305: 'initializing' : truncation from 'double' to 'const float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/nsSVGFilters.cpp(722) : warning C4305: 'initializing' : truncation from 'double' to 'const float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/nsSVGFilters.cpp(722) : warning C4305: 'initializing' : truncation from 'double' to 'const float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/nsSVGFilters.cpp(724) : warning C4305: 'initializing' : truncation from 'double' to 'const float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/nsSVGFilters.cpp(724) : warning C4305: 'initializing' : truncation from 'double' to 'const float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/nsSVGFilters.cpp(724) : warning C4305: 'initializing' : truncation from 'double' to 'const float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/nsSVGFilters.cpp(726) : warning C4305: 'initializing' : truncation from 'double' to 'const float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/nsSVGFilters.cpp(726) : warning C4305: 'initializing' : truncation from 'double' to 'const float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/nsSVGFilters.cpp(727) : warning C4305: 'initializing' : truncation from 'double' to 'const float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/nsSVGFilters.cpp(727) : warning C4305: 'initializing' : truncation from 'double' to 'const float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/nsSVGFilters.cpp(728) : warning C4305: 'initializing' : truncation from 'double' to 'const float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/nsSVGFilters.cpp(728) : warning C4305: 'initializing' : truncation from 'double' to 'const float'

e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/content/svg/content/src/nsSVGFilters.cpp(823) : warning C4305: 'initializing' : truncation from 'double' to 'const float'
}

(I might've missed one, but I think that's all of 'em)
Version: unspecified → Trunk
Comment on attachment 734324 [details] [diff] [review]
patch

># HG changeset patch
># Parent 5346dad80c129071f6c0b2c658dc2c4639bc8145
># User Masatoshi Kimura <VYV03354@nifty.ne.jp>
>Remove the MSVC exemption for FAIL_ON_WARNINGS in content/svg/content/src

"Fix remaining MSVC build warnings and remove [etc]"

>-  UpdateListIndicesFromIndex(aIndex, -(argCount + 1));
>+  UpdateListIndicesFromIndex(aIndex, 0 - (argCount + 1));

This probably merits a brief code comment to explain the "0 - " there.

Maybe: 
 // Note: The subtraction from 0 below is necessary to make the expression
 // signed (and fix MSVC build warning C4146).  Simply negating an unsigned
 // value would not make it signed.

>-  animVal->UpdateListIndicesFromIndex(aIndex, -(1 + aArgCountForItem));
>+  animVal->UpdateListIndicesFromIndex(aIndex, 0 - (1 + aArgCountForItem));

Here as well.

> SVGAElement::SVGAElement(already_AddRefed<nsINodeInfo> aNodeInfo)
>   : SVGAElementBase(aNodeInfo),
>-    Link(this)
>+    ALLOW_THIS_IN_INITIALIZER_LIST(Link(this))

I'm a little confused by the (existing) Link usage here -- from glancing at the Link documentation, it doesn't look like it's expecting to get constructed w/ 'this' as the passed-in arg... I need to glance at this code a bit more to know if this is sane or not.  This isn't the fault of your patch, though, so I suppose this is fine.

>+++ b/content/svg/content/src/SVGLength.cpp
>diff --git a/content/svg/content/src/nsSVGFilters.cpp b/content/svg/content/src/nsSVGFilters.cpp
>--- a/content/svg/content/src/nsSVGFilters.cpp
>+++ b/content/svg/content/src/nsSVGFilters.cpp
>   static const float FACTORx[3][3] =
>-    { { 2.0 / 3.0, 1.0 / 3.0, 2.0 / 3.0 },
>+    { { 2.0f / 3.0f, 1.0f / 3.0f, 2.0f / 3.0f },
>       { 1.0 / 2.0, 1.0 / 4.0, 1.0 / 2.0 },
>-      { 2.0 / 3.0, 1.0 / 3.0, 2.0 / 3.0 } };
>+      { 2.0f / 3.0f, 1.0f / 3.0f, 2.0f / 3.0f } };

Don't you need to tweak that second line, too? (Starting with 1.0 / 2.0)

>-  const float radPerDeg = M_PI/180.0;
>+  const float radPerDeg = static_cast<float>(M_PI/180.0);

I slightly wonder if we should have M_PI_FLOAT for cases like this, so we can just do the math w/ floats up-front instead of doing double math and then downconverting?  I suppose this is OK though.

Anyway, r=me with that the comments added for those "0 -" chunks, and with the 1.0 / 2.0 line fixed w/ "f" suffixes. (assuming it passes Try)
Attachment #734324 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #2)
> >-  UpdateListIndicesFromIndex(aIndex, -(argCount + 1));
> >+  UpdateListIndicesFromIndex(aIndex, 0 - (argCount + 1));
> 
> This probably merits a brief code comment to explain the "0 - " there.
> 
> Maybe: 
>  // Note: The subtraction from 0 below is necessary to make the expression
>  // signed (and fix MSVC build warning C4146).  Simply negating an unsigned
>  // value would not make it signed.

The expression is still unsigned even if the unsigned value is subtracted from 0. (I don't know why MSVC doesn't warn for the binary operator while it does for the unary operator.)
I'll add a comment explaining C4146 anyway.

> >-    { { 2.0 / 3.0, 1.0 / 3.0, 2.0 / 3.0 },
> >+    { { 2.0f / 3.0f, 1.0f / 3.0f, 2.0f / 3.0f },
> >       { 1.0 / 2.0, 1.0 / 4.0, 1.0 / 2.0 },
> >-      { 2.0 / 3.0, 1.0 / 3.0, 2.0 / 3.0 } };
> >+      { 2.0f / 3.0f, 1.0f / 3.0f, 2.0f / 3.0f } };
> 
> Don't you need to tweak that second line, too? (Starting with 1.0 / 2.0)

Because float can express 1.0 / 2.0 and 1.0 / 4.0 precisely. (Recall computers will represent numeric values as binary numbers.) It looks like that MSVC warns only if the downconvertion may actually loose the prevision.
Try result:
https://tbpl.mozilla.org/?tree=Try&rev=a2991f0e8afd
The only modification by the review comment is adding a comment, so I'll land it on inbound.
Sorry, forgot qref before importing the patch...
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e2fa34df9d4
Nice. Thanks for this. :)
https://hg.mozilla.org/mozilla-central/rev/7fe1df8b1a9b
https://hg.mozilla.org/mozilla-central/rev/8e2fa34df9d4
Assignee: nobody → VYV03354
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(In reply to Masatoshi Kimura [:emk] from comment #3)
> > >-    { { 2.0 / 3.0, 1.0 / 3.0, 2.0 / 3.0 },
> > >+    { { 2.0f / 3.0f, 1.0f / 3.0f, 2.0f / 3.0f },
> > >       { 1.0 / 2.0, 1.0 / 4.0, 1.0 / 2.0 },
> > Don't you need to tweak that second line, too? (Starting with 1.0 / 2.0)
> 
> Because float can express 1.0 / 2.0 and 1.0 / 4.0 precisely. (Recall
> computers will represent numeric values as binary numbers.) It looks like
> that MSVC warns only if the downconvertion may actually loose the prevision.

Ah, I see.

Nonetheless, 1.0, 2.0, and 4.0 are still double-precision constants, and we're storing them (or an arithmetic-derived result from them) in a single-precision float value. Even if the compiler handles that and doesn't bother issuing a warning, it's inconsistent & stylistically incorrect to leave off the "f" suffix here. These should be 1.0f, 2.0f, and 4.0f.  (Note also that elsewhere in your patch

I'll push a trivial followup to address this (just adding "f" suffixes) shortly.
(In reply to Daniel Holbert [:dholbert] from comment #9)
> (Note also that elsewhere in your patch

(er, that was going to say "Note also that elsewhere in your patch you've converted 1.0 to 1.0f, even though 1.0 seems likely to be representable precisely as a float value and technically wouldn't need the "f" suffix according to this logic"

(Anyway, that was a tangent. </tangent>)
(In reply to Daniel Holbert [:dholbert] from comment #9)
> These should be 1.0f, 2.0f, and 4.0f.
[...]
> I'll push a trivial followup to address this (just adding "f" suffixes)
> shortly.

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/9bda607c832d
You need to log in before you can comment on or make changes to this bug.