Re: [PATCH v1] travis-ci: override CFLAGS properly, add -Wdeclaration-after-statement

From: Jeff King <peff@peff.net>
To: Lars Schneider <larsxschneider@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	Git Users <git@vger.kernel.org>,
	linux.mdb@gmail.com, Stefan Beller <sbeller@google.com>,
	Jonathan Nieder <jrnieder@gmail.com>
Subject: Re: [PATCH v1] travis-ci: override CFLAGS properly, add -Wdeclaration-after-statement
Date: Tue, 9 Feb 2016 12:36:22 -0500	[thread overview]
Message-ID: <20160209173622.GA28699@sigill.intra.peff.net> (raw)
In-Reply-To: <9FBB5AFB-D44C-43A1-A0C5-A5A45675A122@gmail.com>

On Tue, Feb 09, 2016 at 11:06:17AM +0100, Lars Schneider wrote:

> I collected the warnings from Junio's Make [1] script and merged them with 
> yours. This is the resulting warning list for clang and gcc:
> 
> -Wdeclaration-after-statement -Wno-format-zero-length -Wold-style-definition 
> -Woverflow -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla

Sounds right.

> > I have, however, wondered if it would make sense to codify this
> > knowledge in the Makefile with an optional knob. E.g., let DEVELOPER=1
> > roughly mean "you are a git dev, you have a reasonably modern compiler,
> > and want to be as careful as possible before sending your patches".
> 
> That make sense. However, in "development mode" I don't like Werror.
> How about two knobs? One called DEVELOPER which enables all the warnings
> above and one called CONTINUOUS_INTEGRATION that enables Werror
> in addition?

I'm curious why you don't want -Werror. Junio is going to compile the
result of applying your patch with it, so it makes sense to me to catch
problems as early as possible.

And while there are certainly false positives from gcc's warnings, we
work to squelch them whether -Werror is in effect or not. So IMHO,
-Werror is mostly about making sure you _see_ the warnings and don't
lose them in a flood of compilation messages.

> Regarding CI checks:
> 
> Jeff Merkey made me aware of http://kernelnewbies.org/FirstKernelPatch [2]
> where I found checkpatch.pl [3]. Would it make sense to check all commits
> that are not in next/master/maint with this script on Travis-CI?
> 
> Stefan Beller recently mentioned "Adhere to the common coding style of 
> Git" [4] where he removed explicit NULL checks. This kind of stuff could be
> checked automatically with checkpatch.pl as far as I can see.

Perhaps. I'm not sure that people actually use checkpatch.pl for git.
Out of curiosity, I tried:

  mkdir out
  git format-patch -o out v2.6.0..v2.7.0
  checkpatch.pl out/*

It's rather noisy, and after skimming, I'd say (subjectively) that only
a small fraction are actual style issues we try to enforce. So it would
certainly need a fair bit of tweaking for regular use, I think.

-Peff