On 08 Feb 2016, at 13:25, Jeff King <p...@peff.net> wrote: > On Mon, Feb 08, 2016 at 09:59:18AM +0100, larsxschnei...@gmail.com wrote: > >> From: Lars Schneider <larsxschnei...@gmail.com> >> >> 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
