patchMakefile: move 'ifdef DEVELOPER' after config.mak* inclusion

The DEVELOPER knob was introduced in 658df95 (add DEVELOPER makefile knob to check for acknowledged warnings, 2016-02-25), and works well when used as "make DEVELOPER=1", and when the configure script was not used.

However, the advice given in CodingGuidelines to add DEVELOPER=1 to config.mak does not: config.mak is included after testing for DEVELOPER in the Makefile, and at least GNU Make's manual specifies "Conditional directives are parsed immediately", hence the config.mak declaration is not visible at the time the conditional is evaluated.

Also, when using the configure script to generate a config.mak.autogen, the later file contained a "CFLAGS = <flags>" initialization, which overrode the "CFLAGS += -W..." triggered by DEVELOPER.

This patch fixes both issues.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
I'm surprised that no one noticed the issue. Probably because the
Makefile is silent by default. Did I miss anything obvious?

 Makefile | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

Probably because the overlap of the population that use DEVELOPER=1 and config.mak is miniscule?

When you work in multiple "git worktrees" (or its equivalent), it is far more convenient to have a single "make" wrapper that you use everywhere than to make sure that you copy (or symlink) a config.mak into each and every one of them.

In any case, this change is a right thing to do. Thanks for noticing.

On Tue, May 31, 2016 at 03:24:43PM +0200, Matthieu Moy wrote:

Hmm. So I think this does fix some issues, but it also means that one's config.mak cannot use DEVELOPER as a base and then override particular flags.

I dunno if people want to do that or not. I do not use DEVELOPER myself because I have my own detailed config.mak that is a superset.

-Peff

Jeff King <peff@peff.net> writes:

You mean, using "make DEVELOPER=1" and then tweak CFLAGS in config.mak?

Well, you still can do "CFLAGS += ..." (the extra CFLAGS will come before the ones added by DEVELOPER instead of after), which should cover 99% use-cases.

You can't do "CFLAGS = $(filter-out ..., $(CFLAGS))" anymore indeed. But if you are at that level of customization, I'd say DEVELOPER isn't for you and you should just set CFLAGS directly.

Not really serious, but we can fix that easily. Patch follows.

This does not change the behavior, but allows the user to tweak DEVELOPER_CFLAGS on the command-line or in a config.mak* file if needed.

This also makes the code somewhat cleaner as it follows the pattern

<initialisation of variables> <include statements> <actual build logic>

by specifying which flags to activate in the first part, and actually activating them in the last one.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Junio, you can add this to mm/makefile-developer-can-be-in-config-mak
(or squash it in the commit, but having two separate commit messages
make sense IMO).

 Makefile | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

On Wed, Jun 01, 2016 at 09:57:20AM +0200, Matthieu Moy wrote:

I specifically meant this, that your flags will now come before the DEVELOPER ones. So they will not override for any options which are parsed in command-line order (e.g., -Wno-error=something-specific).

Yes, though it would be nice if the developer cflags were in a separate variable to make that easier to play with.

Perhaps:

  DEVELOPER_CFLAGS += -Wfoo
  DEVELOPER_CFLAGS += -Wbar
  ...
  -include config.mak
  ...
  ifdef DEVELOPER
  CFLAGS += $(DEVELOPER_CFLAGS)
  endif

would be more flexible.

I don't currently use filter-out, but I do have compiler-specific flags (which I accomplish by just not adding them in the first place for certain compilers). For example, you may notice that:

  make DEVELOPER=1 CC=clang

is broken (clang doesn't know -Wold-style-declaration).

-Peff

On Wed, Jun 01, 2016 at 04:03:48AM -0400, Jeff King wrote:

Heh, our mails crossed, but I see you had the same idea. I think it is a strict improvement.

-Peff