Re: [PATCH 4/3] Makefile: untangle DEVELOPER and -Werror

[Duy Nguyen <pclouds@gmail.com>]

On Tue, Apr 03, 2018 at 11:19:46AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> On Sat, Mar 31 2018, Duy Nguyen wrote:
> 
> > On Sat, Mar 31, 2018 at 6:40 PM, Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
> >> Change the DEVELOPER flag, and the newly added EAGER_DEVELOPER flag
> >> which (approximately) enables -Wextra so that any combination of them
> >> and -Werror can be set.
> >>
> >> I've long wanted to use DEVELOPER=1 in my production builds, but on
> >> some old systems I still get warnings, and thus the build would
> >> fail. However if the build/tests fail for some other reason, it would
> >> still be useful to scroll up and see what the relevant code is warning
> >> about.
> >>
> >> This change allows for that. Now setting DEVELOPER will set -Werror as
> >> before, but if DEVELOPER_NONFATAL is set you'll get the same warnings,
> >> but without -Werror.
> >>
> >> I've renamed the newly added EAGER_DEVELOPER flag to
> >> DEVELOPER_EXTRA. The reason is that it approximately turns on -Wextra,
> >> and it'll be more consistent to add e.g. DEVELOPER_PEDANTIC later than
> >> inventing some new name of our own (VERY_EAGER_DEVELOPER?).
> >
> > Before we go with zillions of *DEVELOPER* maybe we can have something
> > like DEVOPTS where you can give multiple keywords to a single variable
> > to influence config.mak.dev. This is similar to COMPILER_FEATURES we
> > already have in there, but now it's driven by the dev instead of the
> > compiler. So you can have keywords like "gentle" (no -Werror) "extra"
> > (-Wextra with no suppression) and something else.
> 
> We could do that, but I don't think it's that bad. This patch is one
> extra option on top of yours,

And that eager this was marked experimental because I was not sure if
it was the right way :)

> and it's not going to result in some combinatorial explosion of
> options, i.e. if we add DEVELOPER_PEDANTIC we'll just add one extra
> flag.
> 
> But sure, we could make this some string we'd need to parse out similar
> to COMPILER_FEATURES, it just seems more complex to me for this task.

It's not that complex. With the EAGER_DEVELOPER patch removed, we can
have something like this where eager devs just need to put

    DEVOPTS = gentle no-suppression

and you put

    DEVOPTS = gentle

(bad naming, I didn't spend time thinking about names)

-- 8< --
diff --git a/Makefile b/Makefile
index e6680a8977..7b4e038e1d 100644
--- a/Makefile
+++ b/Makefile
@@ -435,6 +435,11 @@ all::
 # Define DEVELOPER to enable more compiler warnings. Compiler version
 # and faimily are auto detected, but could be overridden by defining
 # COMPILER_FEATURES (see config.mak.dev)
+#
+# When DEVELOPER is set, DEVOPTS can be used to control compiler options.
+# This variable contains keywords separated by whitespace. Two keywords
+# are recognized: "gentle" removes -Werror and "no-suppression"
+# removes all "-Wno-" options.
 
 GIT-VERSION-FILE: FORCE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
diff --git a/config.mak.dev b/config.mak.dev
index 716a14ecc7..1d7aba6a6a 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -1,4 +1,6 @@
+ifeq ($(filter gentle,$(DEVOPTS)),)
 CFLAGS += -Werror
+endif
 CFLAGS += -Wdeclaration-after-statement
 CFLAGS += -Wno-format-zero-length
 CFLAGS += -Wold-style-definition
@@ -21,6 +23,7 @@ CFLAGS += -Wextra
 # if a function is public, there should be a prototype and the right
 # header file should be included. If not, it should be static.
 CFLAGS += -Wmissing-prototypes
+ifeq ($(filter no-suppression,$(DEVOPTS)),)
 # These are disabled because we have these all over the place.
 CFLAGS += -Wno-empty-body
 CFLAGS += -Wno-missing-field-initializers
@@ -28,6 +31,7 @@ CFLAGS += -Wno-sign-compare
 CFLAGS += -Wno-unused-function
 CFLAGS += -Wno-unused-parameter
 endif
+endif
 
 # uninitialized warnings on gcc 4.9.2 in xdiff/xdiffi.c and config.c
 # not worth fixing since newer compilers correctly stop complaining
-- 8< --