[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] warnings: Add gl_WARN_COMPLEMENT and gl_WARN_SUPPORTED.
From: |
Simon Josefsson |
Subject: |
Re: [PATCH] warnings: Add gl_WARN_COMPLEMENT and gl_WARN_SUPPORTED. |
Date: |
Tue, 18 Nov 2008 14:07:22 +0100 |
User-agent: |
Gnus/5.110011 (No Gnus v0.11) Emacs/22.2 (gnu/linux) |
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