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

[Jeff King <peff@peff.net>]

On Mon, Feb 08, 2016 at 09:59:18AM +0100, larsxschneider@gmail.com wrote:

> From: Lars Schneider <larsxschneider@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 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".

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

-Peff