Enable -Werror in skia on Linux

Enable -Werror in skia.

Fix some harmless warnings.  Also disable strict aliasing on targets
that have the warning.  This will allow us to enable -Werror in
Chromium's skia.gyp.

[bsalomon, 2012-01-05 13:45:06]

On 2012/01/05 13:36:38, reed1 wrote:

I'm all in favor of turning on -Werror (and in the future its equivalent on
windows) but shouldn't we fix the strict aliasing issues rather than use
-fno-strict-aliasing?

[reed1, 2012-01-05 13:49:49]

interesting read on strict-aliasing:
http://cellperformance.beyond3d.com/articles/2006/06/understanding-strict-ali...

[caryclark1, 2012-01-05 13:53:06]

http://codereview.appspot.com/5516044/diff/1/gm/arithmode.cpp
File gm/arithmode.cpp (right):

http://codereview.appspot.com/5516044/diff/1/gm/arithmode.cpp#newcode31
gm/arithmode.cpp:31: SkPoint::Make(SkIntToScalar(WW), SkIntToScalar(HH)) };
Wouldn't a second set of braces work? e.g.
= { {0, 0}, {SkIntToScalar(WW), ...

http://codereview.appspot.com/5516044/diff/1/gm/tablecolorfilter.cpp
File gm/tablecolorfilter.cpp (right):

http://codereview.appspot.com/5516044/diff/1/gm/tablecolorfilter.cpp#newcode23
gm/tablecolorfilter.cpp:23: SkPoint::Make(SkIntToScalar(W), SkIntToScalar(H)) };
again, additional braces instead of a function call seems preferable

http://codereview.appspot.com/5516044/diff/1/gyp/animator.gyp
File gyp/animator.gyp (right):

http://codereview.appspot.com/5516044/diff/1/gyp/animator.gyp#newcode191
gyp/animator.gyp:191: ],
I'd prefer that this be on all platforms, or at least Mac as well, rather than
fragmenting this way -- since it makes regressions by editing on one platform
more likely

http://codereview.appspot.com/5516044/diff/1/gyp/core.gyp
File gyp/core.gyp (right):

http://codereview.appspot.com/5516044/diff/1/gyp/core.gyp#newcode254
gyp/core.gyp:254: # SkScalerContext.cpp break stric aliasing.
s/break stric/breaks strict/

http://codereview.appspot.com/5516044/diff/1/src/effects/SkTableColorFilter.cpp
File src/effects/SkTableColorFilter.cpp (right):

http://codereview.appspot.com/5516044/diff/1/src/effects/SkTableColorFilter.c...
src/effects/SkTableColorFilter.cpp:174: int count = gCountNibBits[fFlags & 0xF];
make 'int count' 'uint8_t count' instead -- note that gCountNibBits is decleared
as uint8_t.

http://codereview.appspot.com/5516044/diff/1/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

http://codereview.appspot.com/5516044/diff/1/src/pdf/SkPDFFont.cpp#newcode476
src/pdf/SkPDFFont.cpp:476: BFRange currentRangeEntry = {0, 0, 0};
= {0} should be sufficient.

http://codereview.appspot.com/5516044/diff/1/tests/EmptyPathTest.cpp
File tests/EmptyPathTest.cpp (right):

http://codereview.appspot.com/5516044/diff/1/tests/EmptyPathTest.cpp#newcode51
tests/EmptyPathTest.cpp:51: bool success = shouldDraw ?
(static_cast<SkPMColor>(~0) == andValue) : (0 == orValue);
change ~0 to ~0U

[TomH, 2012-01-05 16:08:58]

http://codereview.appspot.com/5516044/diff/1/tests/EmptyPathTest.cpp
File tests/EmptyPathTest.cpp (right):

http://codereview.appspot.com/5516044/diff/1/tests/EmptyPathTest.cpp#newcode51
tests/EmptyPathTest.cpp:51: bool success = shouldDraw ?
(static_cast<SkPMColor>(~0) == andValue) : (0 == orValue);
static_cast<SkPMColor>(~0) is starting to look like a magic number - can we
define a named constant for it somewhere instead?

[reed1, 2012-01-05 16:26:32]

http://codereview.appspot.com/5516044/diff/1/src/effects/SkTableColorFilter.cpp
File src/effects/SkTableColorFilter.cpp (right):

http://codereview.appspot.com/5516044/diff/1/src/effects/SkTableColorFilter.c...
src/effects/SkTableColorFilter.cpp:174: int count = gCountNibBits[fFlags & 0xF];
On 2012/01/05 13:53:07, caryclark1 wrote:
> make 'int count' 'uint8_t count' instead -- note that gCountNibBits is
decleared
> as uint8_t.

I think I disagree. count is a local variable to be used in math (trivial as it
is below), and we need not tie its type to the type of that array (which could
change later (tho not likely)).

How about

size_t count = gCountNib...;

This would eliminate the need for the cast below...

[tony, 2012-01-05 18:11:52]

On 2012/01/05 13:45:06, bsalomon wrote:
> On 2012/01/05 13:36:38, reed1 wrote:
> 
> I'm all in favor of turning on -Werror (and in the future its equivalent on
> windows) but shouldn't we fix the strict aliasing issues rather than use
> -fno-strict-aliasing?

Sure, but that should probably be done in follow up patches so we can review
each change more closely.  Also, Chromium compiles with -fno-strict-aliasing so
this makes it no worse than what is already being shipped.

On 2012/01/05 13:53:06, caryclark1 wrote:
> http://codereview.appspot.com/5516044/diff/1/gm/arithmode.cpp
> File gm/arithmode.cpp (right):
> 
> http://codereview.appspot.com/5516044/diff/1/gm/arithmode.cpp#newcode31
> gm/arithmode.cpp:31: SkPoint::Make(SkIntToScalar(WW), SkIntToScalar(HH)) };
> Wouldn't a second set of braces work? e.g.
> = { {0, 0}, {SkIntToScalar(WW), ...

Done.

> http://codereview.appspot.com/5516044/diff/1/gm/tablecolorfilter.cpp
> File gm/tablecolorfilter.cpp (right):
> 
> http://codereview.appspot.com/5516044/diff/1/gm/tablecolorfilter.cpp#newcode23
> gm/tablecolorfilter.cpp:23: SkPoint::Make(SkIntToScalar(W), SkIntToScalar(H))
};
> again, additional braces instead of a function call seems preferable

Done
 
> http://codereview.appspot.com/5516044/diff/1/gyp/animator.gyp
> File gyp/animator.gyp (right):
> 
> http://codereview.appspot.com/5516044/diff/1/gyp/animator.gyp#newcode191
> gyp/animator.gyp:191: ],
> I'd prefer that this be on all platforms, or at least Mac as well, rather than
> fragmenting this way -- since it makes regressions by editing on one platform
> more likely

I agree, but can we enable it one platform at a time?  I imagine clang will have
a few additional warnings that we can fix when enabling on OSX.

> http://codereview.appspot.com/5516044/diff/1/gyp/core.gyp
> File gyp/core.gyp (right):
> 
> http://codereview.appspot.com/5516044/diff/1/gyp/core.gyp#newcode254
> gyp/core.gyp:254: # SkScalerContext.cpp break stric aliasing.
> s/break stric/breaks strict/

Fixed.

>
http://codereview.appspot.com/5516044/diff/1/src/effects/SkTableColorFilter.cpp
> File src/effects/SkTableColorFilter.cpp (right):
> 
>
http://codereview.appspot.com/5516044/diff/1/src/effects/SkTableColorFilter.c...
> src/effects/SkTableColorFilter.cpp:174: int count = gCountNibBits[fFlags &
0xF];
> make 'int count' 'uint8_t count' instead -- note that gCountNibBits is
decleared
> as uint8_t.

Changed to size_t per reed's recommendation.

> http://codereview.appspot.com/5516044/diff/1/src/pdf/SkPDFFont.cpp
> File src/pdf/SkPDFFont.cpp (right):
> 
> http://codereview.appspot.com/5516044/diff/1/src/pdf/SkPDFFont.cpp#newcode476
> src/pdf/SkPDFFont.cpp:476: BFRange currentRangeEntry = {0, 0, 0};
> = {0} should be sufficient.

gcc complains that BFRange::fEnd and BFRange::fUnicode are missing initializers.

> http://codereview.appspot.com/5516044/diff/1/tests/EmptyPathTest.cpp
> File tests/EmptyPathTest.cpp (right):
> 
> http://codereview.appspot.com/5516044/diff/1/tests/EmptyPathTest.cpp#newcode51
> tests/EmptyPathTest.cpp:51: bool success = shouldDraw ?
> (static_cast<SkPMColor>(~0) == andValue) : (0 == orValue);
> change ~0 to ~0U

Done.

On 2012/01/05 16:08:58, TomH wrote:
> http://codereview.appspot.com/5516044/diff/1/tests/EmptyPathTest.cpp
> File tests/EmptyPathTest.cpp (right):
> 
> http://codereview.appspot.com/5516044/diff/1/tests/EmptyPathTest.cpp#newcode51
> tests/EmptyPathTest.cpp:51: bool success = shouldDraw ?
> (static_cast<SkPMColor>(~0) == andValue) : (0 == orValue);
> static_cast<SkPMColor>(~0) is starting to look like a magic number - can we
> define a named constant for it somewhere instead?

~0U worked.  If you think it should be a named constant, please suggest a name
for it.

[tony, 2012-01-06 18:09:31]

ping

[Steve VanDeBogart, 2012-01-06 18:14:48]

LGTM.  reed may have further comment though right now I think he's investigating
a swath of test failures from a skia roll.

[reed1, 2012-01-06 18:15:12]

the .cpp changes look good to me.

I still do not want to disable strict-aliasing until I am clearer on the
performance implications, so do not commit those changes yet.

[TomH, 2012-01-06 18:17:15]

Just to cross-reference, http://codereview.appspot.com/5515047/ discusses strict
referencing.

[tony, 2012-01-06 18:34:19]

New patch with the following changes:
- remove -fno-strict-aliasing
- Commented out -Werror, since it doesn't currently work.  I won't bother
enabling it in chromium until we turn it on here otherwise it'll make rolling
skia a pain.
- Fixed a warning in GrGLInterface.cpp (gcc complained about && || without
parens).
- Fixed a warning in SkAnimateActive.cpp (condition always evaluates to true) by
using a switch statement.

Take another look?

[caryclark1, 2012-01-06 18:39:36]

LGTM

[bsalomon, 2012-01-06 18:40:10]

On 2012/01/06 18:39:36, caryclark1 wrote:
> LGTM

LGTM,too.

[reed1, 2012-01-06 18:41:30]

lgtm, but others should chime in

http://codereview.appspot.com/5516044/diff/16/src/gpu/GrGLInterface.cpp
File src/gpu/GrGLInterface.cpp (right):

http://codereview.appspot.com/5516044/diff/16/src/gpu/GrGLInterface.cpp#newco...
src/gpu/GrGLInterface.cpp:422: if ((kDesktop_GrGLBinding == fBindingsExported &&
If find it very hard to parse this "if" statement now (and before actually).

Brian
1. can you confirm that it is still logically correct with the parens change?
2. can it be reexpressed, or re-indented, to be clearer which predicates go with
which && or || ?

[bsalomon, 2012-01-06 18:59:24]

http://codereview.appspot.com/5516044/diff/16/src/gpu/GrGLInterface.cpp
File src/gpu/GrGLInterface.cpp (right):

http://codereview.appspot.com/5516044/diff/16/src/gpu/GrGLInterface.cpp#newco...
src/gpu/GrGLInterface.cpp:422: if ((kDesktop_GrGLBinding == fBindingsExported &&
On 2012/01/06 18:41:31, reed1 wrote:
> If find it very hard to parse this "if" statement now (and before actually).
> 
> Brian
> 1. can you confirm that it is still logically correct with the parens change?
> 2. can it be reexpressed, or re-indented, to be clearer which predicates go
with
> which && or || ?

Actually the whole thing is wrong: it is missing the return false inside the
conditional. Tony, please just checkin without this change and I will fix it.

[tony, 2012-01-06 19:18:10]

On 2012/01/06 18:59:24, bsalomon wrote:
> Actually the whole thing is wrong: it is missing the return false inside the
> conditional. Tony, please just checkin without this change and I will fix it.

Yay for -Werror finding an actual bug :)

I've uploaded the patch without the change to GrGLInterface.cpp.  I don't have
commit access, so if someone could land it for me, that would be great.

Thanks!

[bsalomon, 2012-01-06 19:22:36]

On 2012/01/06 19:18:10, tony wrote:
> On 2012/01/06 18:59:24, bsalomon wrote:
> > Actually the whole thing is wrong: it is missing the return false inside the
> > conditional. Tony, please just checkin without this change and I will fix
it.
> 
> Yay for -Werror finding an actual bug :)
> 
> I've uploaded the patch without the change to GrGLInterface.cpp.  I don't have
> commit access, so if someone could land it for me, that would be great.
> 
> Thanks!

Committed as r2983.