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

From: Jeff King <peff@peff.net>
Date: Feb 9, 2016, 17:36 UTC
Message-ID: <20160209173622.GA28699@sigill.intra.peff.net>
In-Reply-To: <9FBB5AFB-D44C-43A1-A0C5-A5A45675A122@gmail.com>

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

Show 5 quoted lines

> 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.

Show 9 quoted lines

> > 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.

Show 9 quoted lines

> 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