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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 08 Feb 2016, at 13:25, Jeff King <peff@xxxxxxxx> wrote:

> On Mon, Feb 08, 2016 at 09:59:18AM +0100, larsxschneider@xxxxxxxxx wrote:
> 
>> From: Lars Schneider <larsxschneider@xxxxxxxxx>
>> 
>> The global Travis-CI environment variable CFLAGS did not override the
>> CFLAGS variable in the makefile. Pass CFLAGS as make variable to
>> override it properly.
> 
> Makes sense.
> 
>> In addition to that, add '-Wdeclaration-after-statement' to make a
>> Travis-CI build fail (because of '-Werror') if the code does not adhere
>> to the Git coding style.
> 
> I think this is a good step, but is probably the tip of the iceberg. I
> typically use:
> 
>  -Wall -Werror -Wdeclaration-after-statement
>  -Wpointer-arith -Wstrict-prototypes -Wvla
>  -Wold-style-declaration -Wold-style-definition
> 
> Junio does his integration testing with a similar set, which I think you
> can find in one of the scripts in his "todo" branch.

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

Gcc processes one extra warning that clang doesn't like:
-Wold-style-declaration

The build runs clean with all these checks enabled.

[1] https://github.com/git/git/blob/todo/Make


>> I made this patch because Peff pointed out to me that "git style doesn't
>> allow declaration-after-statement" [1]. I wonder if it would make sense
>> to add this check even in the makefile [2]?
> 
> I think we keep the default CFLAGS to a pretty tame set, so that we
> build out of the box on a large number of compilers. I know I have run
> into problems with -Wold-style-* on clang (though perhaps that is no
> longer the case, as I haven't tried recently), let alone truly antique
> compilers.
> 
> 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 am no make expert, but I
>> also wonder why we don't use the override directive [3] for the CFLAGS?
>> AFAIK this would allow a make invocation like this:
>> 
>> make target CFLAGS+=-Wdeclaration-after-statement
> 
> I think it is rather the opposite. If we used:
> 
>  override CFLAGS+= ...
> 
> in the Makefile, then if a user did:
> 
>  make CFLAGS=...
> 
> we would add to their CFLAGS (whereas without the override, our
> appending is ignored). Our Makefile solves that in a different way,
> though. We pass $(ALL_CFLAGS) to the compiler, and that in turn is made
> up of $(CFLAGS) and $(BASIC_CFLAGS). We assume the user tweaks the
> former, and we set the latter based on Makefile knobs (e.g., NO_CURL,
> etc).
I see. Thanks for the explanation.

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.

[2] http://www.spinics.net/lists/git/msg267445.html
[3] https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl
[4] http://permalink.gmane.org/gmane.comp.version-control.git/280842

Thanks,
Lars
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]