Jim Meyering wrote: > I don't have terribly strong feelings here, but I would be inclined to > retain some of those. > > This exposes low-maint-cost ways to improve APIs, both to help the > compiler help us (e.g., via better optimization or better static > analysis) and to help readers know more at a glance about a labeled > function: > > 72 -Wattribute-warning,
OK, I'm not going to exclude this warning then. > The following are issues that are usually easy/safe to address and > have so few violations that they may be worth addressing or explicitly > suppressing (I haven't looked): > > 7 -Wimplicit-fallthrough= > > 5 -Wmissing-field-initializers OK, I'm going to keep -Wimplicit-fallthrough. And I didn't suggest to exclude -Wmissing-field-initializers warnings. > This one is special: historically low-value, but has been known to > catch real bugs. Mark each FP instance to suppress the warning there? > > 9 -Wmaybe-uninitialized In order to catch these, I'm not excluding the warning from testdirs, only from Gnulib code imported into other packages. We as gnulib maintainers can easily create a testdir, compile it, and analyze all warnings. Paul Eggert wrote: > Like Jim I don't have strong feelings about this, but here are some > comments anyway.... > > > > I therefore propose to disable these warnings: > > > -Wattribute-warning > I haven't dealt with this much, since gcc-warning.spec means it's not > used by the packages I help maintain. Could you give examples of why it > misfires on Gnulib? It often misfires because e.g. strchr (s, ' ') is OK for all multibyte locales but strchr (s, '0') is not. But the attribute-warning can only be attached to a function (here: strchr) as a whole, regardless of which arguments are being passed. > > -Wcast-qual > > -Wconversion > > -Wfloat-conversion > > -Wfloat-equal > > -Wpedantic > > -Wsign-conversion > > -Wundef > > -Wunsuffixed-float-constants > These aren't enabled by -Wall -Wextra. So I assume that gnulib-tool > would be appending -Wno-cast-qual etc. to disable these even if the main > CFLAGS enables them? I want to disable them if the package's AM_CFLAGS enables them. If the user who compiles the package has them enabled, they shall take effect - because "the user is always right". > > -Wimplicit-fallthrough > I typically use -Wimplicit-fallthrough=5; would it be OK to standardize > on that in Gnulib? We can standardize on '-Wimplicit-fallthrough=3'. If we wanted to standardize on '-Wimplicit-fallthrough=5', gperf-generated code would need to use the more modern syntax instead of /*FALLTHROUGH*/. This, in turn, requires a gperf 3.2 release, which is not yet out-of-the-door. > > -Wmaybe-uninitialized > I find this one useful as it catches real bugs. Admittedly it is a pain > because it also has quite a few false alarms. At RMS's suggestion Emacs > does something like this in config.h: > > #ifdef GCC_LINT > # define UNINIT = {0,} > #else > # define UNINIT /* empty */ > #endif > > and something like this in configure.ac: > > AS_IF([test $gl_gcc_warnings = no], > AC_DEFINE([GCC_LINT], [1], [Define to 1 if --enable-gcc-warnings.])) > > and this in source files for variables that would otherwise be warned > about with the latest GCC: > > char *p UNINIT; > > Perhaps we should move this idea into Gnulib? This idea has not met unanimous agreement, IIRC: - Some distro builders insist on building production binaries in the same way as "debugging" binaries. - While some other people think that debugging/linting code should not be included in production binaries. > > -Wrestrict > This one seems like it could find real bugs; could you give an example > or two of misfires? Perhaps we could disable it in individual files that > play nonstandard games with pointers. It does not misfire. It highlights real bugs in crypto code that Simon contributed. OK, I'll not exclude -Wrestrict warnings. Bruno 2022-01-05 Bruno Haible <br...@clisp.org> gnulib-tool: Avoid known warnings that reflect Gnulib's coding style. * m4/gnulib-common.m4 (gl_CC_GNULIB_WARNINGS): New macro. * gnulib-tool (func_emit_lib_Makefile_am): Add the GL_CFLAG_GNULIB_WARNINGS to the CFLAGS of all the compilation units of the library. (func_emit_tests_Makefile_am): Add the GL_CFLAG_GNULIB_WARNINGS to the CFLAGS. (func_import): Emit an invocation of gl_CC_GNULIB_WARNINGS. diff --git a/gnulib-tool b/gnulib-tool index 02d4f8661d..a91847f55a 100755 --- a/gnulib-tool +++ b/gnulib-tool @@ -3974,6 +3974,9 @@ func_emit_lib_Makefile_am () fi echo echo "${libname}_${libext}_SOURCES =" + if ! $for_test; then + echo "${libname}_${libext}_CFLAGS = \$(AM_CFLAGS) \$(GL_CFLAG_GNULIB_WARNINGS)" + fi # Here we use $(LIBOBJS), not @LIBOBJS@. The value is the same. However, # automake during its analysis looks for $(LIBOBJS), not for @LIBOBJS@. echo "${libname}_${libext}_LIBADD = \$(${macro_prefix}_${perhapsLT}LIBOBJS)" @@ -4343,7 +4346,12 @@ func_emit_tests_Makefile_am () # CFLAGS, they have asked for errors, they will get errors. But they have # no right to complain about these errors, because Gnulib does not support # '-Werror'. - echo "CFLAGS = @GL_CFLAG_ALLOW_WARNINGS@ @CFLAGS@" + cflags_for_gnulib_code= + if ! $for_test; then + # Enable or disable warnings as suitable for the Gnulib coding style. + cflags_for_gnulib_code=" \$(GL_CFLAG_GNULIB_WARNINGS)" + fi + echo "CFLAGS = @GL_CFLAG_ALLOW_WARNINGS@${cflags_for_gnulib_code} @CFLAGS@" echo "CXXFLAGS = @GL_CXXFLAG_ALLOW_WARNINGS@ @CXXFLAGS@" echo echo "AM_CPPFLAGS = \\" @@ -6056,6 +6064,7 @@ s,//*$,/,' func_emit_autoconf_snippets "$testsrelated_modules" "$main_modules $testsrelated_modules" func_verify_module true true true echo " m4_popdef([gl_MODULE_INDICATOR_CONDITION])" func_emit_initmacro_end ${macro_prefix}tests $gentests + echo " AC_REQUIRE([gl_CC_GNULIB_WARNINGS])" # _LIBDEPS and _LTLIBDEPS variables are not needed if this library is # created using libtool, because libtool already handles the dependencies. if test "$libtool" != true; then diff --git a/m4/gnulib-common.m4 b/m4/gnulib-common.m4 index 87a9a751b6..179aac7aa1 100644 --- a/m4/gnulib-common.m4 +++ b/m4/gnulib-common.m4 @@ -1,4 +1,4 @@ -# gnulib-common.m4 serial 69 +# gnulib-common.m4 serial 70 dnl Copyright (C) 2007-2022 Free Software Foundation, Inc. dnl This file is free software; the Free Software Foundation dnl gives unlimited permission to copy and/or distribute it, @@ -879,6 +879,70 @@ AC_DEFUN([gl_CXX_ALLOW_WARNINGS], AC_SUBST([GL_CXXFLAG_ALLOW_WARNINGS]) ]) +# gl_CC_GNULIB_WARNINGS +# sets and substitutes a variable GL_CFLAG_GNULIB_WARNINGS, to a $(CC) option +# set that enables or disables warnings as suitable for the Gnulib coding style. +AC_DEFUN([gl_CC_GNULIB_WARNINGS], +[ + AC_REQUIRE([gl_CC_ALLOW_WARNINGS]) + dnl Assume that the compiler supports -Wno-* options only if it also supports + dnl -Wno-error. + GL_CFLAG_GNULIB_WARNINGS='' + if test -n "$GL_CFLAG_ALLOW_WARNINGS"; then + dnl Enable these warning options: + dnl + dnl GCC clang + dnl -Wno-cast-qual >= 3 >= 3.9 + dnl -Wno-conversion >= 3 >= 3.9 + dnl -Wno-float-conversion >= 4.9 >= 3.9 + dnl -Wno-float-equal >= 3 >= 3.9 + dnl -Wimplicit-fallthrough >= 3 >= 3.9 + dnl -Wno-pedantic >= 4.8 >= 3.9 + dnl -Wno-sign-compare >= 3 >= 3.9 + dnl -Wno-sign-conversion >= 4.3 >= 3.9 + dnl -Wno-type-limits >= 4.3 >= 3.9 + dnl -Wno-undef >= 3 >= 3.9 + dnl -Wno-unsuffixed-float-constants >= 4.5 + dnl -Wno-unused-function >= 3 >= 3.9 + dnl -Wno-unused-parameter >= 3 >= 3.9 + dnl + cat > conftest.c <<\EOF + #if __GNUC__ >= 3 || (__clang_major__ + (__clang_minor__ >= 9) > 3) + -Wno-cast-qual + -Wno-conversion + -Wno-float-equal + -Wimplicit-fallthrough + -Wno-sign-compare + -Wno-undef + -Wno-unused-function + -Wno-unused-parameter + #endif + #if __GNUC__ + (__GNUC_MINOR__ >= 9) > 4 || (__clang_major__ + (__clang_minor__ >= 9) > 3) + -Wno-float-conversion + #endif + #if __GNUC__ + (__GNUC_MINOR__ >= 8) > 4 || (__clang_major__ + (__clang_minor__ >= 9) > 3) + -Wno-pedantic + #endif + #if __GNUC__ + (__GNUC_MINOR__ >= 3) > 4 || (__clang_major__ + (__clang_minor__ >= 9) > 3) + -Wno-sign-conversion + -Wno-type-limits + #endif + #if __GNUC__ + (__GNUC_MINOR__ >= 5) > 4 || (__clang_major__ + (__clang_minor__ >= 9) > 3) + -Wno-unsuffixed-float-constants + #endif +EOF + gl_command="$CC $CFLAGS $CPPFLAGS -E conftest.c > conftest.out" + if AC_TRY_EVAL([gl_command]); then + gl_options=`grep -v '#' conftest.out` + for word in $gl_options; do + GL_CFLAG_GNULIB_WARNINGS="$GL_CFLAG_GNULIB_WARNINGS $word" + done + fi + rm -f conftest.c conftest.out + fi + AC_SUBST([GL_CFLAG_GNULIB_WARNINGS]) +]) + dnl gl_CONDITIONAL_HEADER([foo.h]) dnl takes a shell variable GL_GENERATE_FOO_H (with value true or false) as input dnl and produces