Re: [PATCH] warnings: Add gl_WARN_COMPLEMENT and gl_WARN_SUPPORTED.

[Simon Josefsson]

Bruno Haible <address@hidden> writes:

> Simon Josefsson wrote:
>> I'm preparing a blog post about gcc warnings, but briefly my idea with
>> warnings.m4 is to enable all possible warnings, build your project once
>> with -Werror, disable the warnings that cause problems (using
>> gl_WARN_COMPLEMENT), and set up so that the maintainer always build with
>> -Werror plus the warning parameters.  This protects against introducing
>> new code that triggers warning that weren't already triggered by the
>> existing code base.
>
> By doing this, in the end, the developer will have learned a lot about the
> specific warning options of GCC, but hardly have fixed a real bug in the
> code.

Yes, you are probably right here.  Still, learning which warning
parameters are good in practice may still be worthwhile.

> Because, let me repeat it, most of these warnings don't point to
> *bugs*.  They point to a *coding style* different than the one that
> the warning wants to enforce.

Yes.  Some coding styles, that gcc finds acceptable, can lead to build
failures with other compilers though.

For example Sun CC doesn't handle returning a void value (e.g., 'return
foo()' where foo returns void) from a function that returns void.  A gcc
warning about this would be useful, but I'm not sure I've found it yet.

> I agree that learning about the possible warnings that GCC offers and picking
> the right set is something that every GNU project should do once.

Agreed, and this is what we used to do in GnuTLS.

Alas, GCC is a moving target, so the decision becomes obsolete within a
few year.

Revising the set of warning parameters to use is a complex human task.
I'm trying to force gradual automation of it by making it a two-step
simple process: 1) Sync the list of all warnings with latest GCC
manual. 2) Re-compile and add exceptions for the new warning variables,
and if optionally human time is available, review and modify the source
code.

> But what I fear with your approach is:
>
>   1) Many developers will try the warnings one by one, and often come to
>      the same conclusions. For example, you noticed already:
>
>      W="$W -Wsystem-headers"         # Don't let system headers trigger 
> warnings
>      W="$W -Wc++-compat"             # We don't care strongly about C++ 
> compilers
>      W="$W -Woverlength-strings"     # Some of our strings are too large
>      W="$W -Wsign-conversion"        # Too many warnings for now
>      W="$W -Wconversion"             # Too many warnings for now
>      W="$W -Wtraditional"            # Warns on #elif which we use often
>      W="$W -Wtraditional-conversion" # Too many warnings for now
>      W="$W -Wmissing-noreturn"       # Too many warnings for now
>      W="$W -Wunreachable-code"       # Too many false positives
>      W="$W -Wlogical-op"             # Too many false positives
>
>      It is a waste of time to let the gnulib users and the readers of your
>      blog each try these warnings and come to the same conclusion.

Agreed.  I should spend more time on documenting WHY those warnings are
less useful, so others don't have to repeat that task.  Still, not all
warnings are useless for all projects.

>   2) Some people will apply these warnings also to the gnulib part of their
>      project, and report to gnulib the warnings. It will happen often that
>      a warning option has not yet fired on the user's code but he sees it
>      fire on gnulib code; so he will report it.
>      But I don't want to hear about warnings that are about *style*. I do
>      want to hear about warnings that indicate *bugs*. I invite people to
>      report all warnings they see with "gcc -Wall". But if I have to reply
>      to mails with reports about -Wunused or -Wconversion messages, it will
>      take away my time.

I think we could set a gnulib policy about this: compilation with more
warnings than -Wall is not supported.

> I would find it much better if, rather than taking all possible warnings as a
> starting point, you would recommend a reasonable set of warning options.

Good point.  I guess the effort is to document for each warning variable
the code snippet that triggers it, and discuss whether it is a common
warning and how useful it is.

> The reasonable set that I recommend is "-Wall".

I'd like to be able to add more to that list, to help catch build
failures on other systems.

> Now that you have explained it, I see that while gl_WARN_ADD is generally
> useful, gl_WARN_COMPLEMENT and gl_WARN_SUPPORTED are only useful for projects
> which use your approach - which is not widely employed, since you want to
> blog about it. I guess, 70% of the users will want to use gl_WARN_ADD, and
> only 10% or 20% will want to adopt your approach. I therefore vote for moving
> these two macros to a different gnulib module ('manywarnings'?) and
> documenting them in a section of the manual after the one about the 'warnings'
> module.
>
> Btw, gl_WARN_SUPPORTED is a misnomer: it does not test whether the warnings
> are supported; it's gl_WARN_ADD which does this. gl_WARN_ALL_GCC would be a
> better name.

Sure, I've pushed the patch below.

/Simon

>From c00c93e702be0b4ce7d24a66819c9702ff6e4af8 Mon Sep 17 00:00:00 2001
From: Simon Josefsson <address@hidden>
Date: Tue, 18 Nov 2008 13:50:32 +0100
Subject: [PATCH] warnings: Split off some code to new module manywarnings.

---
 ChangeLog            |    9 +++++
 MODULES.html.sh      |    1 +
 m4/manywarnings.m4   |   94 ++++++++++++++++++++++++++++++++++++++++++++++++++
 m4/warnings.m4       |   88 +----------------------------------------------
 modules/manywarnings |   15 ++++++++
 5 files changed, 120 insertions(+), 87 deletions(-)
 create mode 100644 m4/manywarnings.m4
 create mode 100644 modules/manywarnings

diff --git a/ChangeLog b/ChangeLog
index ba2d3d8..00df019 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2008-11-18  Simon Josefsson  <address@hidden>
+
+       * m4/manywarnings.m4: New file with gl_MANYWARN_COMPLEMENT and
+       gl_MANYWARN_ALL_GCC.
+       * m4/warnings.m4: Removed gl_WARN_SUPPORTED and
+       gl_WARN_COMPLEMENT.  Suggested by Bruno Haible <address@hidden>.
+       * modules/manywarnings: New file.
+       * MODULES.html.sh: Mention manywarnings module.
+
 2008-11-18  Bruno Haible  <address@hidden>
 
        * doc/gnulib-tool.texi (Unit tests): New section.
diff --git a/MODULES.html.sh b/MODULES.html.sh
index d3e5c13..b07229d 100755
--- a/MODULES.html.sh
+++ b/MODULES.html.sh
@@ -2973,6 +2973,7 @@ func_all_modules ()
   func_module relocatable-prog-wrapper
   func_module relocatable-script
   func_module warnings
+  func_module manywarnings
   func_end_table
 
   element="Support for building documentation"
diff --git a/m4/manywarnings.m4 b/m4/manywarnings.m4
new file mode 100644
index 0000000..5eb5a66
--- /dev/null
+++ b/m4/manywarnings.m4
@@ -0,0 +1,94 @@
+# manywarnings.m4 serial 1
+dnl Copyright (C) 2008 Free Software Foundation, Inc.
+dnl This file is free software; the Free Software Foundation
+dnl gives unlimited permission to copy and/or distribute it,
+dnl with or without modifications, as long as this notice is preserved.
+
+dnl From Simon Josefsson
+
+# gl_MANYWARN_COMPLEMENT(OUTVAR, LISTVAR, REMOVEVAR)
+# --------------------------------------------------
+# Copy LISTVAR to OUTVAR except for the entries in REMOVEVAR.
+# Elements separated by whitespace.  In set logic terms, the function
+# does OUTVAR = LISTVAR \ REMOVEVAR.
+AC_DEFUN([gl_COMPLEMENT],
+[
+  gl_warn_set=
+  set x $2; shift
+  for gl_warn_item
+  do
+    case " $3 " in
+      *" $gl_warn_item "*)
+        ;;
+      *)
+        gl_warn_set="$gl_warn_set $gl_warn_item"
+        ;;
+    esac
+  done
+  $1=$gl_warn_set
+])
+
+# gl_MANYWARN_ALL_GCC(VARIABLE)
+# -----------------------------
+# Add all documented GCC (currently as per version 4.3.2) warning
+# parameters to variable VARIABLE.  Note that you need to test them
+# using gl_WARN_ADD if you want to make sure your gcc understands it.
+AC_DEFUN([gl_MANYWARN_ALL_GCC],
+[
+ gl_manywarn_set=
+ for gl_manywarn_item in \
+   -Wall \
+   -W \
+   -Wformat-y2k \
+   -Wformat-nonliteral \
+   -Wformat-security \
+   -Winit-self \
+   -Wmissing-include-dirs \
+   -Wswitch-default \
+   -Wswitch-enum \
+   -Wunused \
+   -Wunknown-pragmas \
+   -Wstrict-aliasing \
+   -Wstrict-overflow \
+   -Wsystem-headers \
+   -Wfloat-equal \
+   -Wtraditional \
+   -Wtraditional-conversion \
+   -Wdeclaration-after-statement \
+   -Wundef \
+   -Wshadow \
+   -Wunsafe-loop-optimizations \
+   -Wpointer-arith \
+   -Wbad-function-cast \
+   -Wc++-compat \
+   -Wcast-qual \
+   -Wcast-align \
+   -Wwrite-strings \
+   -Wconversion \
+   -Wsign-conversion \
+   -Wlogical-op \
+   -Waggregate-return \
+   -Wstrict-prototypes \
+   -Wold-style-definition \
+   -Wmissing-prototypes \
+   -Wmissing-declarations \
+   -Wmissing-noreturn \
+   -Wmissing-format-attribute \
+   -Wpacked \
+   -Wpadded \
+   -Wredundant-decls \
+   -Wnested-externs \
+   -Wunreachable-code \
+   -Winline \
+   -Winvalid-pch \
+   -Wlong-long \
+   -Wvla \
+   -Wvolatile-register-var \
+   -Wdisabled-optimization \
+   -Wstack-protector \
+   -Woverlength-strings \
+  ; do
+    gl_manywarn_set="$gl_manywarn_set $gl_manywarn_item"
+  done
+  $1=$gl_manywarn_set
+])
diff --git a/m4/warnings.m4 b/m4/warnings.m4
index 3585c3e..c32cf4e 100644
--- a/m4/warnings.m4
+++ b/m4/warnings.m4
@@ -1,4 +1,4 @@
-# warnings.m4 serial 1
+# warnings.m4 serial 2
 dnl Copyright (C) 2008 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -42,89 +42,3 @@ AS_VAR_POPDEF([gl_Flags])dnl
 AS_VAR_POPDEF([gl_Warn])dnl
 m4_ifval([$2], [AS_LITERAL_IF([$2], [AC_SUBST([$2])], [])])dnl
 ])
-
-# gl_WARN_SUPPORTED(VARIABLE)
-# ----------------------
-# Add all supported warning parameters to variable VARIABLE.
-AC_DEFUN([gl_WARN_SUPPORTED],
-[
- FOO=
- # List of all supported warning parameters according to GCC 4.3.2 manual.
- for w in \
-   -Wall \
-   -W \
-   -Wformat-y2k \
-   -Wformat-nonliteral \
-   -Wformat-security \
-   -Winit-self \
-   -Wmissing-include-dirs \
-   -Wswitch-default \
-   -Wswitch-enum \
-   -Wunused \
-   -Wunknown-pragmas \
-   -Wstrict-aliasing \
-   -Wstrict-overflow \
-   -Wsystem-headers \
-   -Wfloat-equal \
-   -Wtraditional \
-   -Wtraditional-conversion \
-   -Wdeclaration-after-statement \
-   -Wundef \
-   -Wshadow \
-   -Wunsafe-loop-optimizations \
-   -Wpointer-arith \
-   -Wbad-function-cast \
-   -Wc++-compat \
-   -Wcast-qual \
-   -Wcast-align \
-   -Wwrite-strings \
-   -Wconversion \
-   -Wsign-conversion \
-   -Wlogical-op \
-   -Waggregate-return \
-   -Wstrict-prototypes \
-   -Wold-style-definition \
-   -Wmissing-prototypes \
-   -Wmissing-declarations \
-   -Wmissing-noreturn \
-   -Wmissing-format-attribute \
-   -Wpacked \
-   -Wpadded \
-   -Wredundant-decls \
-   -Wnested-externs \
-   -Wunreachable-code \
-   -Winline \
-   -Winvalid-pch \
-   -Wlong-long \
-   -Wvla \
-   -Wvolatile-register-var \
-   -Wdisabled-optimization \
-   -Wstack-protector \
-   -Woverlength-strings \
-  ; do
-    FOO="$FOO $w"
-  done
-  $1=$FOO
-])
-
-# gl_WARN_COMPLEMENT(OUTVAR, LISTVAR, REMOVEVAR)
-# ----------------------
-# Copy LISTVAR to OUTVAR except for the entries in REMOVEVAR.
-# Elements separated by whitespace.  In set logic terms, the function
-# does OUTVAR = LISTVAR \ REMOVEVAR.
-AC_DEFUN([gl_WARN_COMPLEMENT],
-[
-  gl_warn_set=
-  set x $2; shift
-  for gl_warn_item
-  do
-    case " $3 " in
-      *" $gl_warn_item "*)
-        ;;
-      *)
-        gl_warn_set="$gl_warn_set $gl_warn_item"
-        ;;
-    esac
-  done
-  $1=$gl_warn_set
-])
diff --git a/modules/manywarnings b/modules/manywarnings
new file mode 100644
index 0000000..fae9221
--- /dev/null
+++ b/modules/manywarnings
@@ -0,0 +1,15 @@
+Description:
+Helper M4 functions to help work out a set of warning parameters to use.
+
+Files:
+m4/manywarnings.m4
+
+Depends-on:
+
+configure.ac:
+
+License:
+unlimited
+
+Maintainer:
+Simon Josefsson
-- 
1.5.6.5