Re: stat-time.h vs. -Waggregate-return

[Jim Meyering]

Jim Meyering wrote:
> Simon Josefsson wrote:
>> Eric Blake <address@hidden> writes:
>>> On 08/02/2012 04:49 PM, Paul Eggert wrote:
>>>> On 08/02/2012 03:40 PM, Eric Blake wrote:
>>>>> /* Store *ST's access time into *TS.  */
>>>>> static inline void
>>>>> get_stat_atime (struct stat const *st, struct timespec *ts)
>>>>
>>>> I'd rather not go that route, as the functional style is easier
>>>> to read -- in particular, it tends to avoid aliasing issues.
>>>>
>>>> In the GNU apps I'm familiar with (Emacs, coreutils, ...)
>>>> we simply disable -Waggregate-return.  It a completely
>>>> anachronistic warning, since its motivation was to
>>>> support backwards compatibility with C compilers that
>>>> did not allow returning structures.  Those compilers
>>>> are long dead and are no longer of practical concern.
>>>
>>> Fair enough; that argues that 'manywarnings' should be customized to
>>> easily ditch this and other anachronistic warnings (for example,
>>> -Wtraditional, -Wlong-long) in one line, rather than making every single
>>> client repeat the same list of customizations.
>>
>> I agree the current situation could be improved, and we have touched on
>> this before.  I would prefer to solve this with a two-step approach:
>>
>> 1) The goal of the manywarnings module should be to discover ALL warning
>> flags that the compiler supports, whether the warning message is useful
>> or not [as long as it doesn't break builds].  Having the complete list
>> of warnings a compiler support available can be useful for experimenting
>> with various coding styles.
>>
>> 2) There could be a 'reasonablewarnings' module that depended on
>> manywarnings, which would do further filtering of the warning list to
>> limit it to warning flags relevant to GNU-style project.  This module
>> could remove flags like -Wtraditional, -Wsystem-headers and others which
>> we consider useless for the majority of projects.
>>
>> What do you think?
>>
>> (maybe the module names should be improved though)
>
> I worked on this about a year ago, and have just refreshed
> my list and filter against the latest from upstream gcc/master.
> Here's what I suggest:
>
> Periodically run this with the very latest gcc in your path.
> Assuming you have a cloned gnulib directory in $gl, it will print
> out any options that have been added to gcc that are not yet on our list:
>
>     comm -1 -3 \
>       <(sed -n 's/^  *\(-[^ ]*\) .*/\1/p' $gl/m4/manywarnings.m4 |sort) \
>       <(gcc --help=warnings|sed -n 's/^  \(-[^ ]*\) .*/\1/p' |sort \
>         |grep -v --line-regexp -f <(cut -f1 $gl/build-aux/gcc-warning.spec))
>
> That working depends on a new file, tentatively named
> build-aux/gcc-warning.spec, that tells how to classify
> these warnings (currently binary: use it, or don't).
>
> Here's the file, most of which I wrote months ago.  Considering the
> number of FIXME notes, you can see that we are almost certain to want
> more than two categories, and probably multiple tags on the RHS.  Some of
> the FIXME comments reflect the simple fact that I didn't have a lot of
> time to read-the-documentation/understand and experiment with each of
> those options.  Having multiple tags will allow packages to customize
> filtering, e.g., to enable all c++-related warnings, but to exclude
> warnings that are deemed obsolescent or too strict.
>
> Suggestions welcome.
>
>>From 51d2b564fe80e603f4fb801b7265db9c27ce91ac Mon Sep 17 00:00:00 2001
> From: Jim Meyering <address@hidden>
> Date: Wed, 30 Nov 2011 14:25:35 +0100
> Subject: [PATCH] manywarnings: update the list of "all" warnings

No one replied, so I've updated the two lists from the latest
gcc (built from git/svn yesterday) and have tested the result
by building coreutils, grep and diffutils using that new
set of warnings.  Only one small change was needed to make
coreutils compile with --enable-gcc-warnings:
[though the factor complaint was about vfprintf, which you'd think
would have the attribute in glibc's stdio.h -- it does not ]

    src/factor.c: In function 'debug':
    src/factor.c:62:7: error: function might be possible candidate for 
'gnu_printf' format attribute [-Werror=suggest-attribute=format]
           vfprintf (stderr, fmt, ap);
           ^
    cc1: all warnings being treated as errors
    make: *** [src/factor.o] Error 1


diff --git a/configure.ac b/configure.ac
index 51782a5..627920d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -128,6 +128,7 @@ if test "$gl_gcc_warnings" = yes; then
   nw="$nw -Wmissing-format-attribute" # copy.c
   nw="$nw -Wunsafe-loop-optimizations" # a few src/*.c
   nw="$nw -Winline"                 # system.h's 
readdir_ignoring_dot_and_dotdot
+  nw="$nw -Wsuggest-attribute=format" # warns about copy.c and factor.c

   # Using -Wstrict-overflow is a pain, but the alternative is worse.
   # For an example, see the code that provoked this report:

I made one change to grep:

  http://thread.gmane.org/gmane.comp.gnu.grep.bugs/4664

and diffutils required no change.

>From dd44da552f3f158a55b04fbe11ef1d0faf5ee5ba Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Wed, 30 Nov 2011 14:25:35 +0100
Subject: [PATCH] manywarnings: update the list of "all" warnings

* m4/manywarnings.m4: Unite lists, and add many new options.
* build-aux/gcc-warning: New file.

Run this command with the latest gcc to see if they have added
options not yet on our list:

gl=.; comm -1 -3 \
  <(sed -n 's/^  *\(-[^ ]*\) .*/\1/p' $gl/m4/manywarnings.m4 |sort) \
  <(gcc --help=warnings|sed -n 's/^  \(-[^ ]*\) .*/\1/p' |sort \
    |grep -v --line-regexp -f <(cut -f1 $gl/build-aux/gcc-warning.spec))
---
 build-aux/gcc-warning.spec |  85 +++++++++++++++++++++++++
 m4/manywarnings.m4         | 153 ++++++++++++++++++++++++++-------------------
 2 files changed, 173 insertions(+), 65 deletions(-)
 create mode 100644 build-aux/gcc-warning.spec

diff --git a/build-aux/gcc-warning.spec b/build-aux/gcc-warning.spec
new file mode 100644
index 0000000..74c6503
--- /dev/null
+++ b/build-aux/gcc-warning.spec
@@ -0,0 +1,85 @@
+# options to filter out, and why
+--all-warnings                         alias for -Wall
+--extra-warnings                       alias for -Wextra
+-Waggregate-return                     obsolescent
+-Waliasing                             fortran
+-Walign-commons                                fortran
+-Wampersand                            fortran
+-Warray-temporaries                    fortran
+-Wassign-intercept                     objc/objc++
+-Wc++-compat                           FIXME maybe? borderline.  some will 
want this
+-Wc++0x-compat                         c++
+-Wc++11-compat                         c++
+-Wc-binding-type                       fortran
+-Wc-binding-type                        fortran
+-Wcast-qual                            FIXME maybe? too much noise; encourages 
bad changes
+-Wcharacter-truncation                 fortran
+-Wcompare-reals                         fortran
+-Wconversion                           FIXME maybe? too much noise; encourages 
bad changes
+-Wconversion-extra                     fortran
+-Wconversion-null                      c++ and objc++
+-Wctor-dtor-privacy                    c++
+-Wdeclaration-after-statement          FIXME: do not want.  others may
+-Wdeclaration-after-statement           obsolescent
+-Wdelete-non-virtual-dtor              c++
+-Weffc++                               c++
+-Werror-implicit-function-declaration  deprecated
+-Wfloat-equal                          FIXME maybe? borderline.  some will 
want this
+-Wformat                               covered by -Wformat=2
+-Wformat=                              gcc --help=warnings artifact
+-Wfunction-elimination                 fortran
+-Wimplicit-interface                   fortran
+-Wimplicit-procedure                   fortran
+-Wintrinsic-shadow                     fortran
+-Wintrinsics-std                       fortran
+-Winvalid-offsetof                     c++ and objc++
+-Wlarger-than-                         gcc --help=warnings artifact
+-Wlarger-than=<number>                 FIXME: choose something sane?
+-Wline-truncation                      fortran
+-Wliteral-suffix                       c++ and objc++
+-Wliteral-suffix                        c++ and objc++
+-Wlong-long                            obsolescent
+-Wnoexcept                             c++
+-Wnon-template-friend                  c++
+-Wnon-virtual-dtor                     c++
+-Wnormalized=<id|nfc|nfkc>             FIXME: choose something sane?
+-Wold-style-cast                       c++ and objc++
+-Woverloaded-virtual                   c++
+-Wpadded                               FIXME: dunno
+-Wpadded                                FIXME maybe?  warns about "stabil" 
member in /usr/include/bits/timex.h
+-Wpedantic                             FIXME: too strict?
+-Wpmf-conversions                      c++ and objc++
+-Wproperty-assign-default              objc++
+-Wprotocol                             objc++
+-Wreal-q-constant                      fortran
+-Wrealloc-lhs                          fortran
+-Wrealloc-lhs                           fortran
+-Wrealloc-lhs-all                      fortran
+-Wrealloc-lhs-all                       fortran
+-Wredundant-decls                      FIXME maybe? many _gl_cxxalias_dummy FPs
+-Wreorder                              c++ and objc++
+-Wselector                             objc and objc++
+-Wsign-compare                         FIXME maybe? borderline.  some will 
want this
+-Wsign-conversion                      FIXME maybe? borderline.  some will 
want this
+-Wsign-promo                           c++ and objc++
+-Wstack-usage=                         FIXME: choose something sane?
+-Wstrict-aliasing=                     FIXME: choose something sane?
+-Wstrict-null-sentinel                 c++ and objc++
+-Wstrict-overflow=                     FIXME: choose something sane?
+-Wstrict-selector-match                        objc and objc++
+-Wsurprising                           fortran
+-Wswitch-enum                          FIXME maybe? borderline.  some will 
want this
+-Wsynth                                        deprecated
+-Wtabs                                 fortran
+-Wtarget-lifetime                       fortran
+-Wtraditional                          obsolescent
+-Wtraditional-conversion               obsolescent
+-Wundeclared-selector                  objc and objc++
+-Wundef                                        FIXME maybe? too many false 
positives
+-Wunderflow                            fortran
+-Wunsuffixed-float-constants            triggers warning in gnulib's timespec.h
+-Wunused-dummy-argument                        fortran
+-Wuseless-cast                         c++ and objc++
+-Wuseless-cast                          c++ and objc++
+-Wzero-as-null-pointer-constant                c++ and objc++
+-frequire-return-statement             go
diff --git a/m4/manywarnings.m4 b/m4/manywarnings.m4
index 864fc85..2760efb 100644
--- a/m4/manywarnings.m4
+++ b/m4/manywarnings.m4
@@ -81,95 +81,118 @@ 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 \
+    -Wabi \
+    -Waddress \
+    -Wall \
+    -Warray-bounds \
+    -Wattributes \
     -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 \
     -Wbuiltin-macro-redefined \
-    -Wmudflap \
-    -Wpacked-bitfield-compat \
-    -Wsync-nand \
-    ; do
-    gl_manywarn_set="$gl_manywarn_set $gl_manywarn_item"
-  done
-  # The following are not documented in the manual but are included in
-  # output from gcc --help=warnings.
-  for gl_manywarn_item in \
-    -Wattributes \
+    -Wcast-align \
+    -Wchar-subscripts \
+    -Wclobbered \
+    -Wcomment \
+    -Wcomments \
     -Wcoverage-mismatch \
-    -Wunused-macros \
-    ; do
-    gl_manywarn_set="$gl_manywarn_set $gl_manywarn_item"
-  done
-  # More warnings from gcc 4.6.2 --help=warnings.
-  for gl_manywarn_item in \
-    -Wabi \
     -Wcpp \
     -Wdeprecated \
     -Wdeprecated-declarations \
+    -Wdisabled-optimization \
     -Wdiv-by-zero \
     -Wdouble-promotion \
+    -Wempty-body \
     -Wendif-labels \
+    -Wenum-compare \
     -Wextra \
     -Wformat-contains-nul \
     -Wformat-extra-args \
+    -Wformat-nonliteral \
+    -Wformat-security \
+    -Wformat-y2k \
     -Wformat-zero-length \
     -Wformat=2 \
+    -Wfree-nonheap-object \
+    -Wignored-qualifiers \
+    -Wimplicit \
+    -Wimplicit-function-declaration \
+    -Wimplicit-int \
+    -Winit-self \
+    -Winline \
+    -Wint-to-pointer-cast \
+    -Winvalid-memory-model \
+    -Winvalid-pch \
+    -Wjump-misses-init \
+    -Wlogical-op \
+    -Wmain \
+    -Wmaybe-uninitialized \
+    -Wmissing-braces \
+    -Wmissing-declarations \
+    -Wmissing-field-initializers \
+    -Wmissing-format-attribute \
+    -Wmissing-include-dirs \
+    -Wmissing-noreturn \
+    -Wmissing-parameter-type \
+    -Wmissing-prototypes \
+    -Wmudflap \
     -Wmultichar \
+    -Wnarrowing \
+    -Wnested-externs \
+    -Wnonnull \
     -Wnormalized=nfc \
+    -Wold-style-declaration \
+    -Wold-style-definition \
     -Woverflow \
+    -Woverlength-strings \
+    -Woverride-init \
+    -Wpacked \
+    -Wpacked-bitfield-compat \
+    -Wparentheses \
+    -Wpointer-arith \
+    -Wpointer-sign \
     -Wpointer-to-int-cast \
     -Wpragmas \
+    -Wreturn-type \
+    -Wsequence-point \
+    -Wshadow \
+    -Wsizeof-pointer-memaccess \
+    -Wstack-protector \
+    -Wstrict-aliasing \
+    -Wstrict-overflow \
+    -Wstrict-prototypes \
     -Wsuggest-attribute=const \
+    -Wsuggest-attribute=format \
     -Wsuggest-attribute=noreturn \
     -Wsuggest-attribute=pure \
+    -Wswitch \
+    -Wswitch-default \
+    -Wsync-nand \
+    -Wsystem-headers \
     -Wtrampolines \
+    -Wtrigraphs \
+    -Wtype-limits \
+    -Wuninitialized \
+    -Wunknown-pragmas \
+    -Wunreachable-code \
+    -Wunsafe-loop-optimizations \
+    -Wunused \
+    -Wunused-but-set-parameter \
+    -Wunused-but-set-variable \
+    -Wunused-function \
+    -Wunused-label \
+    -Wunused-local-typedefs \
+    -Wunused-macros \
+    -Wunused-parameter \
+    -Wunused-result \
+    -Wunused-value \
+    -Wunused-variable \
+    -Wvarargs \
+    -Wvariadic-macros \
+    -Wvector-operation-performance \
+    -Wvla \
+    -Wvolatile-register-var \
+    -Wwrite-strings \
+    \
     ; do
     gl_manywarn_set="$gl_manywarn_set $gl_manywarn_item"
   done
--
1.7.12.146.g16d26b1