[PATCH libinput 2/2] enable -Wall -Werror for CFLAGS

[Peter Hutterer]

On 21/08/2015 21:04 , Andreas Pokorny wrote:
> Hi,
>
> The sole purpose of Werror is to keep warnings out. But libinput already
> did a good job without Wall + Werror being enabled.

yeah, fwiw, the general rule is that patches that introduce warnings 
won't get merged.

>
> On Fri, Aug 21, 2015 at 4:05 AM, Peter Hutterer
> <peter.hutterer at who-t.net <mailto:peter.hutterer at who-t.net>> wrote:
>
>     On Thu, Aug 20, 2015 at 02:32:23PM +0300, Pekka Paalanen wrote:
>      > On Thu, 20 Aug 2015 12:51:38 +0200
>      > Andreas Pokorny <andreas.pokorny at canonical.com
>     <mailto:andreas.pokorny at canonical.com>> wrote:
>      >
>      > > Just a small change in test is necessary to enable -Wall -Werror.
>      > >
>      > > Signed-off-by: Andreas Pokorny <andreas.pokorny at canonical.com
>     <mailto:andreas.pokorny at canonical.com>>
>      > > ---
>      > > configure.ac <http://configure.ac>  | 4 ++--
>      > >  test/litest.c | 8 ++++++--
>      > >  2 files changed, 8 insertions(+), 4 deletions(-)
>      > >
>      > > diff --git a/configure.ac <http://configure.ac> b/configure.ac
>     <http://configure.ac>
>      > > index 885cb39..b7597f0 100644
>      > > --- a/configure.ac <http://configure.ac>
>      > > +++ b/configure.ac <http://configure.ac>
>      > > @@ -87,8 +87,8 @@ AC_CHECK_LIB([m], [atan2])
>      > >  AC_CHECK_LIB([rt], [clock_gettime])
>      > >
>      > >  if test "x$GCC" = "xyes"; then
>      > > -   GCC_CXXFLAGS="-Wall -Wextra -Wno-unused-parameter -g
>     -fvisibility=hidden"
>      > > -   GCC_CFLAGS="$GCC_CXXFLAGS -Wmissing-prototypes
>     -Wstrict-prototypes"
>      > > +   GCC_CXXFLAGS="-Wall -Werror -Wextra -Wno-unused-parameter
>     -g -fvisibility=hidden"
>      > > +   GCC_CFLAGS="$GCC_CXXFLAGS -Wall -Werror
>     -Wmissing-prototypes -Wstrict-prototypes"
>      >
>      > Hi,
>      >
>      > are you sure you want to force -Werror on everyone? Even distribution
>      > builds?
>
>
> Rather force it on the project. I think that developers are usually
> ahead of distributions in terms of compiler versions.

but you can't guarantee that the warnings you see are the same as 
someone else does. Sometimes you get different warnings depending on 
whether you use -O0 or -O2, you can get different warnings for different 
versions of your dependencies. case in point: I have -Wall in my $CFLAGS 
yet the warning you fixed never showed up. So either you have an older 
or a newer header file.

as I said above, we effectively enforce a no-warning policy anyway, but 
it does require them to trigger on one of the machines I run the builds on.

>      > New compilers come out with new warnings, new or old headers might
>      > cause warnings...
>      >
>      > While checking with colleagues, I was pointed at
>      > https://www.gnu.org/software/autoconf-archive/ax_compiler_flags.html
>      > Maybe that could inspire something?
>      >
>
>     yeah, I agree with Pekka here. -Werror is almost always wrong unless you
>     added it yourself to the $CFLAGS on the host you're building on.
>     Worse, -Werror doesn't actually add anything - the reason these warnings
>     haven't been caught before was because they didn't show up. some of them
>     depend on compiler versions, library versions, etc.
>
>
> The second warning showed up because I added -Wall which before was only
> present in the CXXFLAGS.

from configure.ac:
	GCC_CFLAGS="$GCC_CXXFLAGS -Wmissing-prototypes -Wstrict-prototypes"

so -Wall should always be enabled, right?

> You get something out of Werror if you enforce it, and sure Werror does
> not add additional warnings.

no, but it can 'arbitrarily' break the build, and the more dependencies 
you have the greater the risk. That's ok when you're planning for it, 
but not so much when the packager doesn't know how to fix the issue 
immediately. And as I said above, by having it in configure.ac you're 
forcing the packagers to patch it out rather than just set/unset an 
environment variable.

So by all means, put -Werror into your local $CFLAGS, but let's not 
enforce it on downstreams.

Cheers,
   Peter

>
>     AX_COMPILER_FLAGS looks interesting, but unsure on the result. the term
>     "useful warnings" can be quite project-dependent and adding #pragmas
>     to turn
>     some of them off isn't helpful either. Haven't tried it yet though.
>
>
>     Merged patch 1/2 though, thanks.
>
>
> Thanks.
>
> regards
> Andreas