clang++ 11 compilation issues

clang++ 11 compilation issues

[Alexandre Duret-Lutz]

Hi,

I'm upgrading a project that used a 8-month-old copy of gnulib to
today's version, and testing the result with various compilers.

This is a C++17 project that is not internationalized and that
uses the following modules:
  argmatch
  argp
  closeout
  error
  isatty
  mkstemp
  mkstemps
  progname
  secure_getenv
  stpcpy
  strverscmp
  sys_wait

My copy of gnulib is also patched with a patch I sent in 2013:
https://lists.gnu.org/archive/html/bug-gnulib/2013-12/msg00106.html

I compile with -Werror.

Testing the result with clang++ 11.0.1-2 (Debian unstable), I observe
the following two issues:

(1) lib/argmatch.h includes lib/gettext.h which fails as follows

> clang++ -DHAVE_CONFIG_H -I. -I..  -I.. -I.. -I../buddy/src -I../lib -I../lib  -W -Wall -Werror -Wint-to-void-pointer-cast -Wzero-as-null-pointer-constant -Wcast-align -Wpointer-arith -Wwrite-strings -Wcast-qual -DXTSTRINGDEFINES -Wdocumentation -Wmissing-declarations -Woverloaded-virtual -Wmisleading-indentation -Wimplicit-fallthrough -Wnull-dereference -Wsuggest-override -Wpedantic -fvisibility=hidden -fvisibility-inlines-hidden -DSPOT_BUILD -std=c++17 -g -O -MT autcross.o -MD -MP -MF .deps/autcross.Tpo -c -o autcross.o autcross.cc
> In file included from autcross.cc:34:
> In file included from ../lib/argmatch.h:31:
> ../lib/gettext.h:234:22: error: zero as null pointer constant [-Werror,-Wzero-as-null-pointer-constant]
>   if (msg_ctxt_id != NULL)
>                      ^~~~
>                      nullptr
> /usr/lib/llvm-11/lib/clang/11.0.1/include/stddef.h:84:18: note: expanded from macro 'NULL'
> #    define NULL __null
>                  ^
> In file included from autcross.cc:34:
> In file included from ../lib/argmatch.h:31:
> ../lib/gettext.h:282:22: error: zero as null pointer constant [-Werror,-Wzero-as-null-pointer-constant]
>   if (msg_ctxt_id != NULL)
>                      ^~~~
>                      nullptr
> /usr/lib/llvm-11/lib/clang/11.0.1/include/stddef.h:84:18: note: expanded from macro 'NULL'
> #    define NULL __null
>                  ^
> 2 errors generated.

Just replacing these two tests with "if (msg_ctxt_id)" gets rid of the issue.


(2) ARGMATCH_VERIFY ends up using _Static_assert which is a C11 keyword
    that does not exist in C++.  However static_assert exists in C++11
    with two args, and since C++17 with one arg.

> clang++ -DHAVE_CONFIG_H -I. -I..  -I.. -I.. -I../buddy/src -I../lib -I../lib  -W -Wall -Werror -Wint-to-void-pointer-cast -Wzero-as-null-pointer-constant -Wcast-align -Wpointer-arith -Wwrite-strings -Wcast-qual -DXTSTRINGDEFINES -Wdocumentation -Wmissing-declarations -Woverloaded-virtual -Wmisleading-indentation -Wimplicit-fallthrough -Wnull-dereference -Wsuggest-override -Wpedantic -fvisibility=hidden -fvisibility-inlines-hidden -DSPOT_BUILD -std=c++17 -g -O -MT common_aoutput.o -MD -MP -MF .deps/common_aoutput.Tpo -c -o common_aoutput.o common_aoutput.cc
> common_aoutput.cc:85:1: error: '_Static_assert' is a C11 extension [-Werror,-Wc11-extensions]
> ARGMATCH_VERIFY(check_args, check_types);
> ^
> ../lib/argmatch.h:45:5: note: expanded from macro 'ARGMATCH_VERIFY'
>     verify (ARRAY_CARDINALITY (Arglist) == ARRAY_CARDINALITY (Vallist) + 1)
>     ^
> ../lib/verify.h:289:20: note: expanded from macro 'verify'
> # define verify(R) _GL_VERIFY (R, "verify (" #R ")", -)
>                    ^
> ../lib/verify.h:229:41: note: expanded from macro '_GL_VERIFY'
> # define _GL_VERIFY(R, DIAGNOSTIC, ...) _Static_assert (R, DIAGNOSTIC)
>                                         ^
> common_aoutput.cc:397:7: error: '_Static_assert' is a C11 extension [-Werror,-Wc11-extensions]
>       ARGMATCH_VERIFY(args, format);
>       ^
> ../lib/argmatch.h:45:5: note: expanded from macro 'ARGMATCH_VERIFY'
>     verify (ARRAY_CARDINALITY (Arglist) == ARRAY_CARDINALITY (Vallist) + 1)
>     ^
> ../lib/verify.h:289:20: note: expanded from macro 'verify'
> # define verify(R) _GL_VERIFY (R, "verify (" #R ")", -)
>                    ^
> ../lib/verify.h:229:41: note: expanded from macro '_GL_VERIFY'
> # define _GL_VERIFY(R, DIAGNOSTIC, ...) _Static_assert (R, DIAGNOSTIC)
>                                         ^
> 2 errors generated.

to work around this, I've just changed the definition of _GL_VERIFY in
lib/verify.h from

> #if defined _GL_HAVE__STATIC_ASSERT
> # define _GL_VERIFY(R, DIAGNOSTIC, ...) _Static_assert (R, DIAGNOSTIC)
> #else
> # define _GL_VERIFY(R, DIAGNOSTIC, ...)                                \
>     extern int (*_GL_GENSYM (_gl_verify_function) (void))	       \
>       [_GL_VERIFY_TRUE (R, DIAGNOSTIC)]
> #endif

to

> #if defined __cpp_static_assert
> # define _GL_VERIFY(R, DIAGNOSTIC, ...) static_assert (R, DIAGNOSTIC)
> #elif defined _GL_HAVE__STATIC_ASSERT
> # define _GL_VERIFY(R, DIAGNOSTIC, ...) _Static_assert (R, DIAGNOSTIC)
> #else
> # define _GL_VERIFY(R, DIAGNOSTIC, ...)                                \
>     extern int (*_GL_GENSYM (_gl_verify_function) (void))	       \
>       [_GL_VERIFY_TRUE (R, DIAGNOSTIC)]
> #endif

However there are a few __cplusplus tests at the top of the file that
attempt to tell when _Static_assert can be used (and failed here), and I
do not really follow that logic.  Those macros check for
__cpp_static_assert to assume something about _Static_assert, which
seems dubious.

-- 
Alexandre Duret-Lutz

Re: clang++ 11 compilation issues

[Florian Weimer]

* Alexandre Duret-Lutz:

> (1) lib/argmatch.h includes lib/gettext.h which fails as follows
>
>> clang++ -DHAVE_CONFIG_H -I. -I..  -I.. -I.. -I../buddy/src -I../lib -I../lib  -W -Wall -Werror -Wint-to-void-pointer-cast -Wzero-as-null-pointer-constant -Wcast-align -Wpointer-arith -Wwrite-strings -Wcast-qual -DXTSTRINGDEFINES -Wdocumentation -Wmissing-declarations -Woverloaded-virtual -Wmisleading-indentation -Wimplicit-fallthrough -Wnull-dereference -Wsuggest-override -Wpedantic -fvisibility=hidden -fvisibility-inlines-hidden -DSPOT_BUILD -std=c++17 -g -O -MT autcross.o -MD -MP -MF .deps/autcross.Tpo -c -o autcross.o autcross.cc
>> In file included from autcross.cc:34:
>> In file included from ../lib/argmatch.h:31:
>> ../lib/gettext.h:234:22: error: zero as null pointer constant [-Werror,-Wzero-as-null-pointer-constant]
>>   if (msg_ctxt_id != NULL)
>>                      ^~~~
>>                      nullptr
>> /usr/lib/llvm-11/lib/clang/11.0.1/include/stddef.h:84:18: note: expanded from macro 'NULL'
>> #    define NULL __null
>>                  ^

Would you be able to check whether __null is in the preprocessed
sources?  If it is there (and the lack of further logged expansions
suggests this), then this is a compiler bug.  __null is not zero, and
should be fine to use as a null pointer constant.  This is why NULL is
not defined as 0.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: clang++ 11 compilation issues

[Bruno Haible]

Hi Alexandre,

> I'm upgrading a project that used a 8-month-old copy of gnulib to
> today's version

Since then, gnulib has improved its support for clang.

> I compile with -Werror.

Gnulib generally does not support -Werror on arbitrary platforms.

We try to make for a warning-free compilation on glibc systems with GCC,
as far as it is reasonable (but it does not work always, due to GCC bugs).

> Testing the result with clang++ 11.0.1-2 (Debian unstable)

Now, using clang with -Wall and -Werror is a recipe for failures, for sure.

This is because clang has _many_ warnings, and

  - Some of the warnings are of the kind "tell me when you did a certain
    optimization". Most of the warnings in this category are not helpful.

  - Some of the warnings merely enforce certain coding styles. You can
    try to enforce your preferred coding style on your code, but enforcing
    it on Gnulib code is a non-starter.

  - Some of the warnings are contradictory.

You are supposed to choose the warnings that are reasonable for your
project. For some project of mine, I had to disable 20-40 warning options
before I could get reasonable output.

> > ../lib/gettext.h:234:22: error: zero as null pointer constant [-Werror,-Wzero-as-null-pointer-constant]
> >   if (msg_ctxt_id != NULL)
> >                      ^~~~

This will not be changed. NULL is the preferred way to denote a null pointer.

> Just replacing these two tests with "if (msg_ctxt_id)" gets rid of the issue.

... but makes the code harder to understand.

> (2) ARGMATCH_VERIFY ends up using _Static_assert which is a C11 keyword
>     that does not exist in C++.  However static_assert exists in C++11
>     with two args, and since C++17 with one arg.

We prefer to use _Static_assert with 2 arguments, since it causes the compiler
to give the diagnostic specified by the programmer. A macro or built-in
that accepts only 1 argument is an inferior solution for this case.

> > clang++ -DHAVE_CONFIG_H -I. -I..  -I.. -I.. -I../buddy/src -I../lib -I../lib  -W -Wall -Werror -Wint-to-void-pointer-cast -Wzero-as-null-pointer-constant -Wcast-align -Wpointer-arith -Wwrite-strings -Wcast-qual -DXTSTRINGDEFINES -Wdocumentation -Wmissing-declarations -Woverloaded-virtual -Wmisleading-indentation -Wimplicit-fallthrough -Wnull-dereference -Wsuggest-override -Wpedantic -fvisibility=hidden -fvisibility-inlines-hidden -DSPOT_BUILD -std=c++17 -g -O -MT common_aoutput.o -MD -MP -MF .deps/common_aoutput.Tpo -c -o common_aoutput.o common_aoutput.cc
> > common_aoutput.cc:85:1: error: '_Static_assert' is a C11 extension [-Werror,-Wc11-extensions]
> > ARGMATCH_VERIFY(check_args, check_types);
> > ^
> > ../lib/argmatch.h:45:5: note: expanded from macro 'ARGMATCH_VERIFY'
> >     verify (ARRAY_CARDINALITY (Arglist) == ARRAY_CARDINALITY (Vallist) + 1)
> >     ^
> > ../lib/verify.h:289:20: note: expanded from macro 'verify'
> > # define verify(R) _GL_VERIFY (R, "verify (" #R ")", -)
> >                    ^
> > ../lib/verify.h:229:41: note: expanded from macro '_GL_VERIFY'
> > # define _GL_VERIFY(R, DIAGNOSTIC, ...) _Static_assert (R, DIAGNOSTIC)
> >                                         ^
> > common_aoutput.cc:397:7: error: '_Static_assert' is a C11 extension [-Werror,-Wc11-extensions]
> >       ARGMATCH_VERIFY(args, format);
> >       ^
> > ../lib/argmatch.h:45:5: note: expanded from macro 'ARGMATCH_VERIFY'
> >     verify (ARRAY_CARDINALITY (Arglist) == ARRAY_CARDINALITY (Vallist) + 1)
> >     ^
> > ../lib/verify.h:289:20: note: expanded from macro 'verify'
> > # define verify(R) _GL_VERIFY (R, "verify (" #R ")", -)
> >                    ^
> > ../lib/verify.h:229:41: note: expanded from macro '_GL_VERIFY'
> > # define _GL_VERIFY(R, DIAGNOSTIC, ...) _Static_assert (R, DIAGNOSTIC)
> >                                         ^
> > 2 errors generated.
> 
> to work around this, I've just changed the definition of _GL_VERIFY in
> lib/verify.h from
> 
> > #if defined _GL_HAVE__STATIC_ASSERT
> > # define _GL_VERIFY(R, DIAGNOSTIC, ...) _Static_assert (R, DIAGNOSTIC)
> > #else
> > # define _GL_VERIFY(R, DIAGNOSTIC, ...)                                \
> >     extern int (*_GL_GENSYM (_gl_verify_function) (void))	       \
> >       [_GL_VERIFY_TRUE (R, DIAGNOSTIC)]
> > #endif
> 
> to
> 
> > #if defined __cpp_static_assert
> > # define _GL_VERIFY(R, DIAGNOSTIC, ...) static_assert (R, DIAGNOSTIC)
> > #elif defined _GL_HAVE__STATIC_ASSERT
> > # define _GL_VERIFY(R, DIAGNOSTIC, ...) _Static_assert (R, DIAGNOSTIC)
> > #else
> > # define _GL_VERIFY(R, DIAGNOSTIC, ...)                                \
> >     extern int (*_GL_GENSYM (_gl_verify_function) (void))	       \
> >       [_GL_VERIFY_TRUE (R, DIAGNOSTIC)]
> > #endif

Thanks for the suggestion. Committed through the patch below.

> However there are a few __cplusplus tests at the top of the file that
> attempt to tell when _Static_assert can be used (and failed here), and I
> do not really follow that logic.

These lines define our own witnesses whether a certain feature is available,
rather than merely relying on __cpp_static_assert. This allows us to
cater with compilers which lie about their features (e.g. HP compilers
are frequently broken in this way) or consider GCC's -pedantic option.

> Those macros check for
> __cpp_static_assert to assume something about _Static_assert, which
> seems dubious.

Paul can tell more about this one.


2021-01-12  Bruno Haible  <bruno@clisp.org>

	verify: Use C++11 static_assert when available.
	Reported by Alexandre Duret-Lutz <adl@lrde.epita.fr> in
	<https://lists.gnu.org/archive/html/bug-gnulib/2021-01/msg00177.html>.
	* lib/verify.h (_GL_HAVE_STATIC_ASSERT_CXX11): New macro.
	(_GL_HAVE_STATIC_ASSERT_CXX17): Renamed from _GL_HAVE_STATIC_ASSERT1.
	(_GL_VERIFY): Use static_assert when available with C++11 syntax.

diff --git a/lib/verify.h b/lib/verify.h
index 3cdcdca..a9e7589 100644
--- a/lib/verify.h
+++ b/lib/verify.h
@@ -29,7 +29,11 @@
    per C2X.  This is supported by GCC 9.1 and later, and by clang in
    C++1z mode.
 
-   Define _GL_HAVE_STATIC_ASSERT1 if static_assert (R) works as per
+   Define _GL_HAVE_STATIC_ASSERT_CXX11 if static_assert (R, DIAGNOSTIC)
+   works as per C++11.  This is supported by GCC 6.1 and later, and by
+   clang in C++11 mode.
+
+   Define _GL_HAVE_STATIC_ASSERT_CXX17 if static_assert (R) works as per
    C++17.  This is supported by GCC 9.1 and later, and by clang in
    C++1z mode.
 
@@ -54,10 +58,15 @@
 # if 4 <= __clang_major__ && 201411 <= __cpp_static_assert
 #  define _GL_HAVE__STATIC_ASSERT1 1
 # endif
+# if 201103L <= __cplusplus \
+     || 6 <= __GNUC__ \
+     || (4 <= __clang_major__ && 200410 <= __cpp_static_assert)
+#  define _GL_HAVE_STATIC_ASSERT_CXX11 1
+# endif
 # if 201703L <= __cplusplus \
      || 9 <= __GNUC__ \
      || (4 <= __clang_major__ && 201411 <= __cpp_static_assert)
-#  define _GL_HAVE_STATIC_ASSERT1 1
+#  define _GL_HAVE_STATIC_ASSERT_CXX17 1
 # endif
 #endif
 
@@ -225,7 +234,9 @@ template <int w>
    Unfortunately, unlike C11, this implementation must appear as an
    ordinary declaration, and cannot appear inside struct { ... }.  */
 
-#if defined _GL_HAVE__STATIC_ASSERT
+#if defined _GL_HAVE_STATIC_ASSERT_CXX11
+# define _GL_VERIFY(R, DIAGNOSTIC, ...) static_assert (R, DIAGNOSTIC)
+#elif defined _GL_HAVE__STATIC_ASSERT
 # define _GL_VERIFY(R, DIAGNOSTIC, ...) _Static_assert (R, DIAGNOSTIC)
 #else
 # define _GL_VERIFY(R, DIAGNOSTIC, ...)                                \
@@ -239,7 +250,7 @@ template <int w>
 #  define _Static_assert(...) \
      _GL_VERIFY (__VA_ARGS__, "static assertion failed", -)
 # endif
-# if !defined _GL_HAVE_STATIC_ASSERT1 && !defined static_assert
+# if !defined _GL_HAVE_STATIC_ASSERT_CXX17 && !defined static_assert
 #  define static_assert _Static_assert /* C11 requires this #define.  */
 # endif
 #endif

Re: clang++ 11 compilation issues

[Alexandre Duret-Lutz]

Florian Weimer <fweimer@redhat.com> writes:

> Would you be able to check whether __null is in the preprocessed
> sources?  If it is there (and the lack of further logged expansions
> suggests this), then this is a compiler bug.  __null is not zero, and
> should be fine to use as a null pointer constant.  This is why NULL is
> not defined as 0.

I can reproduce this without any preprocessing with clang 9, 10, and 11.

% cat foo.cc
int foo(char* msg_ctxt_id)
{
  return msg_ctxt_id == __null;
}
% clang++-11 -Wzero-as-null-pointer-constant -c foo.cc
foo.cc:3:25: warning: zero as null pointer constant [-Wzero-as-null-pointer-constant]
  return msg_ctxt_id == __null;
                        ^~~~~~
                        nullptr
1 warning generated.

% clang++-10 -Wzero-as-null-pointer-constant -c foo.cc
foo.cc:3:25: warning: zero as null pointer constant [-Wzero-as-null-pointer-constant]
  return msg_ctxt_id == __null;
                        ^~~~~~
                        nullptr
1 warning generated.
% clang++-9 -Wzero-as-null-pointer-constant -c foo.cc
foo.cc:3:25: warning: zero as null pointer constant [-Wzero-as-null-pointer-constant]
  return msg_ctxt_id == __null;
                        ^~~~~~
                        nullptr
1 warning generated.

-- 
Alexandre Duret-Lutz

Re: clang++ 11 compilation issues

[Jeffrey Walton]

On Tue, Jan 12, 2021 at 3:19 PM Bruno Haible <bruno@clisp.org> wrote:
>
> Hi Alexandre,
>
> > I'm upgrading a project that used a 8-month-old copy of gnulib to
> > today's version
>
> Since then, gnulib has improved its support for clang.
>
> > I compile with -Werror.
>
> Gnulib generally does not support -Werror on arbitrary platforms.
>
> We try to make for a warning-free compilation on glibc systems with GCC,
> as far as it is reasonable (but it does not work always, due to GCC bugs).
>
> > Testing the result with clang++ 11.0.1-2 (Debian unstable)
>
> Now, using clang with -Wall and -Werror is a recipe for failures, for sure.
>
> This is because clang has _many_ warnings, and
>
>   - Some of the warnings are of the kind "tell me when you did a certain
>     optimization". Most of the warnings in this category are not helpful.
>
>   - Some of the warnings merely enforce certain coding styles. You can
>     try to enforce your preferred coding style on your code, but enforcing
>     it on Gnulib code is a non-starter.
>
>   - Some of the warnings are contradictory.
>
> You are supposed to choose the warnings that are reasonable for your
> project. For some project of mine, I had to disable 20-40 warning options
> before I could get reasonable output.

Perhaps it would be a good idea to filter-out the options that you
don't want present for Gnulib.

If you are doing it during configure, then take the user's CFLAGS (or
CXXFLAGS) and then:

       TCFLAGS=`echo $CFLAGS | sed -e 's/-Wall//g' -e 's/-Wextra//g'
-e 's/-Werror//g'`

If you are doing it during make, then use a recipe like this for Gnulib sources:

    GL_CFLAGS := $(filter-out -Wall -Wextra -Werror% -Wunused
-Wconversion -Wp%, $(CFLAGS))
    ...
    %.o:%.c:
        $CC $(strip $CPPFLAGS $GL_CFLAGS -c) $<

That will put an end to these mailing list messages and bug reports.
You get what you want, and users get what they want.

Otherwise, this is an exercise in insanity. Users keep doing the same
thing, GNU keeps doing the same thing, but everyone expects a
different outcome. Instead of practicing inanity, engineer a fix for
the problem.

Jeff

Re: clang++ 11 compilation issues

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 648 bytes --]

On 1/12/21 12:19 PM, Bruno Haible wrote:
>> Those macros check for
>> __cpp_static_assert to assume something about _Static_assert, which
>> seems dubious.
> Paul can tell more about this one.

I agree it's dubious.

Also, looking over the current verify.h, it's a little complicated and I 
doubt whether the complexity is worth it nowadays. How about if we 
simplify the C++ case, by simply relying on __cpp_static_assert there? 
Although this could miss opportunities for generating diagnostics via 
older C++ compilers, I doubt whether they're worth the trouble any more.

Proposed Gnulib patch attached (I haven't installed it).

[-- Attachment #2: 0001-verify-rely-more-on-__cpp_static_assert.patch --]
[-- Type: text/x-patch, Size: 4765 bytes --]

From ce90a689fc388c4c612861054adcb09bd3658b1c Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 13 Jan 2021 15:46:33 -0800
Subject: [PATCH] verify: rely more on __cpp_static_assert

* lib/verify.h (_GL_HAVE__STATIC_ASSERT, _GL_HAVE__STATIC_ASSERT1):
Simplify for the C++ case, by relying only on __cpp_static_assert.
That should be good enough nowadays for C++.
(_GL_HAVE_STATIC_ASSERT_CXX11, _GL_HAVE_STATIC_ASSERT_CXX17):
Remove.  All uses replaced by simply checking __cpp_static_assert.
---
 ChangeLog    |  9 +++++++++
 lib/verify.h | 53 ++++++++++++----------------------------------------
 2 files changed, 21 insertions(+), 41 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 2123974c8..8991d6a35 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2021-01-13  Paul Eggert  <eggert@cs.ucla.edu>
+
+	verify: simplify static_assert configuration
+	* lib/verify.h (_GL_HAVE__STATIC_ASSERT, _GL_HAVE__STATIC_ASSERT1):
+	Do not define for C++.  This should be good enough nowadays,
+	since recent-enough C++ compilers have static_assert.
+	(_GL_HAVE_STATIC_ASSERT_CXX11, _GL_HAVE_STATIC_ASSERT_CXX17):
+	Remove.  All uses replaced by simply checking __cpp_static_assert.
+
 2021-01-13  Simon Josefsson  <simon@josefsson.org>
 
 	lib-msvc-compat: Update libtool usage recommendation.
diff --git a/lib/verify.h b/lib/verify.h
index a9e75890f..a62a47087 100644
--- a/lib/verify.h
+++ b/lib/verify.h
@@ -22,52 +22,23 @@
 
 
 /* Define _GL_HAVE__STATIC_ASSERT to 1 if _Static_assert (R, DIAGNOSTIC)
-   works as per C11.  This is supported by GCC 4.6.0 and later, in C
-   mode, and by clang (also in C++ mode).
+   works as per C11.  This is supported by GCC 4.6.0+ and by clang 4+.
 
    Define _GL_HAVE__STATIC_ASSERT1 to 1 if _Static_assert (R) works as
-   per C2X.  This is supported by GCC 9.1 and later, and by clang in
-   C++1z mode.
-
-   Define _GL_HAVE_STATIC_ASSERT_CXX11 if static_assert (R, DIAGNOSTIC)
-   works as per C++11.  This is supported by GCC 6.1 and later, and by
-   clang in C++11 mode.
-
-   Define _GL_HAVE_STATIC_ASSERT_CXX17 if static_assert (R) works as per
-   C++17.  This is supported by GCC 9.1 and later, and by clang in
-   C++1z mode.
+   per C2X.  This is supported by GCC 9.1+.
 
    Support compilers claiming conformance to the relevant standard,
    and also support GCC when not pedantic.  If we were willing to slow
    'configure' down we could also use it with other compilers, but
    since this affects only the quality of diagnostics, why bother?  */
-#ifndef __cplusplus
-# if (201112L <= __STDC_VERSION__ \
-      || (!defined __STRICT_ANSI__ \
-          && (4 < __GNUC__ + (6 <= __GNUC_MINOR__) || 4 <= __clang_major__)))
-#  define _GL_HAVE__STATIC_ASSERT 1
-# endif
-# if (202000L <= __STDC_VERSION__ \
-      || (!defined __STRICT_ANSI__ && 9 <= __GNUC__))
-#  define _GL_HAVE__STATIC_ASSERT1 1
-# endif
-#else
-# if 4 <= __clang_major__
-#  define _GL_HAVE__STATIC_ASSERT 1
-# endif
-# if 4 <= __clang_major__ && 201411 <= __cpp_static_assert
-#  define _GL_HAVE__STATIC_ASSERT1 1
-# endif
-# if 201103L <= __cplusplus \
-     || 6 <= __GNUC__ \
-     || (4 <= __clang_major__ && 200410 <= __cpp_static_assert)
-#  define _GL_HAVE_STATIC_ASSERT_CXX11 1
-# endif
-# if 201703L <= __cplusplus \
-     || 9 <= __GNUC__ \
-     || (4 <= __clang_major__ && 201411 <= __cpp_static_assert)
-#  define _GL_HAVE_STATIC_ASSERT_CXX17 1
-# endif
+#if (201112L <= __STDC_VERSION__ \
+     || (!defined __STRICT_ANSI__ \
+         && (4 < __GNUC__ + (6 <= __GNUC_MINOR__) || 4 <= __clang_major__)))
+# define _GL_HAVE__STATIC_ASSERT 1
+#endif
+#if (202000L <= __STDC_VERSION__ \
+     || (!defined __STRICT_ANSI__ && 9 <= __GNUC__))
+# define _GL_HAVE__STATIC_ASSERT1 1
 #endif
 
 /* FreeBSD 9.1 <sys/cdefs.h>, included by <stddef.h> and lots of other
@@ -234,7 +205,7 @@ template <int w>
    Unfortunately, unlike C11, this implementation must appear as an
    ordinary declaration, and cannot appear inside struct { ... }.  */
 
-#if defined _GL_HAVE_STATIC_ASSERT_CXX11
+#if 200410 <= __cpp_static_assert
 # define _GL_VERIFY(R, DIAGNOSTIC, ...) static_assert (R, DIAGNOSTIC)
 #elif defined _GL_HAVE__STATIC_ASSERT
 # define _GL_VERIFY(R, DIAGNOSTIC, ...) _Static_assert (R, DIAGNOSTIC)
@@ -250,7 +221,7 @@ template <int w>
 #  define _Static_assert(...) \
      _GL_VERIFY (__VA_ARGS__, "static assertion failed", -)
 # endif
-# if !defined _GL_HAVE_STATIC_ASSERT_CXX17 && !defined static_assert
+# if __cpp_static_assert < 201411 && !defined static_assert
 #  define static_assert _Static_assert /* C11 requires this #define.  */
 # endif
 #endif
-- 
2.27.0

Re: clang++ 11 compilation issues

[Bruno Haible]

Hi Paul,

> Also, looking over the current verify.h, it's a little complicated and I 
> doubt whether the complexity is worth it nowadays. How about if we 
> simplify the C++ case, by simply relying on __cpp_static_assert there? 
> Although this could miss opportunities for generating diagnostics via 
> older C++ compilers, I doubt whether they're worth the trouble any more.

The risk here is not that the new code misses opportunities for generating
diagnostics. The risk here is that the new code introduces compilation errors
in C++ mode.

Recall that in this area C and C++ are very different; which is why the
code starts with a '#if[n]def __cplusplus'. If you try to handle C and C++
through the same code and conditions, it is not only possibly buggy, but
also certainly unmaintainable.

For example, say, someone use g++ version >= 5 with option -std=c++98. Then,
with your new code, __cpp_static_assert will be undefined,
_GL_HAVE__STATIC_ASSERT will be 1, and _GL_VERIFY(R, DIAGNOSTIC, ...) will
expand to
  _Static_assert (R, DIAGNOSTIC)
which most likely leads to a syntax error.

Also note that the C++ test coverage of this module is currently zero (none).
This means that if you make this change and it is buggy, we have no way to
catch it, before some user will stumble across it.

Bruno

Re: clang++ 11 compilation issues

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 1149 bytes --]

On 1/13/21 5:21 PM, Bruno Haible wrote:
> For example, say, someone use g++ version >= 5 with option -std=c++98. Then,
> with your new code, __cpp_static_assert will be undefined,
> _GL_HAVE__STATIC_ASSERT will be 1, and _GL_VERIFY(R, DIAGNOSTIC, ...) will
> expand to
>    _Static_assert (R, DIAGNOSTIC)
> which most likely leads to a syntax error.

Thanks, I see your point. (I had to use -std=gnu++98 to observe the 
problem.)

How about the attached, more-conservative patch instead? It limits 
__cpp_static_assert to controlling the use of 'static_assert', and 
causes C++ code to not use _Static_assert at all.

> Also note that the C++ test coverage of this module is currently zero (none).
> This means that if you make this change and it is buggy, we have no way to
> catch it, before some user will stumble across it.

The users in this case are developers, and the failure mode is merely a 
trivial failure to compile, so these problems should be relatively 
minor. And when things go wrong (as they likely will regardless of 
whether this patch is installed :-), having a simpler verify.h will ease 
users' jobs.

[-- Attachment #2: 0001-verify-simplify-static_assert-configuration.patch --]
[-- Type: text/x-patch, Size: 4038 bytes --]

From bedf196eeebe249f75f71a51a55edca5da78e027 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 13 Jan 2021 15:46:33 -0800
Subject: [PATCH] verify: simplify static_assert configuration

* lib/verify.h (_GL_HAVE__STATIC_ASSERT, _GL_HAVE__STATIC_ASSERT1):
Do not define for C++.  This should be good enough nowadays,
since recent-enough C++ compilers have static_assert.
(_GL_HAVE_STATIC_ASSERT_CXX11, _GL_HAVE_STATIC_ASSERT_CXX17):
Remove.  All uses replaced by simply checking __cpp_static_assert.
---
 ChangeLog    |  9 +++++++++
 lib/verify.h | 35 ++++-------------------------------
 2 files changed, 13 insertions(+), 31 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 2123974c8..8991d6a35 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2021-01-13  Paul Eggert  <eggert@cs.ucla.edu>
+
+	verify: simplify static_assert configuration
+	* lib/verify.h (_GL_HAVE__STATIC_ASSERT, _GL_HAVE__STATIC_ASSERT1):
+	Do not define for C++.  This should be good enough nowadays,
+	since recent-enough C++ compilers have static_assert.
+	(_GL_HAVE_STATIC_ASSERT_CXX11, _GL_HAVE_STATIC_ASSERT_CXX17):
+	Remove.  All uses replaced by simply checking __cpp_static_assert.
+
 2021-01-13  Simon Josefsson  <simon@josefsson.org>
 
 	lib-msvc-compat: Update libtool usage recommendation.
diff --git a/lib/verify.h b/lib/verify.h
index a9e75890f..65514c34b 100644
--- a/lib/verify.h
+++ b/lib/verify.h
@@ -22,20 +22,10 @@
 
 
 /* Define _GL_HAVE__STATIC_ASSERT to 1 if _Static_assert (R, DIAGNOSTIC)
-   works as per C11.  This is supported by GCC 4.6.0 and later, in C
-   mode, and by clang (also in C++ mode).
+   works as per C11.  This is supported by GCC 4.6.0+ and by clang 4+.
 
    Define _GL_HAVE__STATIC_ASSERT1 to 1 if _Static_assert (R) works as
-   per C2X.  This is supported by GCC 9.1 and later, and by clang in
-   C++1z mode.
-
-   Define _GL_HAVE_STATIC_ASSERT_CXX11 if static_assert (R, DIAGNOSTIC)
-   works as per C++11.  This is supported by GCC 6.1 and later, and by
-   clang in C++11 mode.
-
-   Define _GL_HAVE_STATIC_ASSERT_CXX17 if static_assert (R) works as per
-   C++17.  This is supported by GCC 9.1 and later, and by clang in
-   C++1z mode.
+   per C2X.  This is supported by GCC 9.1+.
 
    Support compilers claiming conformance to the relevant standard,
    and also support GCC when not pedantic.  If we were willing to slow
@@ -51,23 +41,6 @@
       || (!defined __STRICT_ANSI__ && 9 <= __GNUC__))
 #  define _GL_HAVE__STATIC_ASSERT1 1
 # endif
-#else
-# if 4 <= __clang_major__
-#  define _GL_HAVE__STATIC_ASSERT 1
-# endif
-# if 4 <= __clang_major__ && 201411 <= __cpp_static_assert
-#  define _GL_HAVE__STATIC_ASSERT1 1
-# endif
-# if 201103L <= __cplusplus \
-     || 6 <= __GNUC__ \
-     || (4 <= __clang_major__ && 200410 <= __cpp_static_assert)
-#  define _GL_HAVE_STATIC_ASSERT_CXX11 1
-# endif
-# if 201703L <= __cplusplus \
-     || 9 <= __GNUC__ \
-     || (4 <= __clang_major__ && 201411 <= __cpp_static_assert)
-#  define _GL_HAVE_STATIC_ASSERT_CXX17 1
-# endif
 #endif
 
 /* FreeBSD 9.1 <sys/cdefs.h>, included by <stddef.h> and lots of other
@@ -234,7 +207,7 @@ template <int w>
    Unfortunately, unlike C11, this implementation must appear as an
    ordinary declaration, and cannot appear inside struct { ... }.  */
 
-#if defined _GL_HAVE_STATIC_ASSERT_CXX11
+#if 200410 <= __cpp_static_assert
 # define _GL_VERIFY(R, DIAGNOSTIC, ...) static_assert (R, DIAGNOSTIC)
 #elif defined _GL_HAVE__STATIC_ASSERT
 # define _GL_VERIFY(R, DIAGNOSTIC, ...) _Static_assert (R, DIAGNOSTIC)
@@ -250,7 +223,7 @@ template <int w>
 #  define _Static_assert(...) \
      _GL_VERIFY (__VA_ARGS__, "static assertion failed", -)
 # endif
-# if !defined _GL_HAVE_STATIC_ASSERT_CXX17 && !defined static_assert
+# if __cpp_static_assert < 201411 && !defined static_assert
 #  define static_assert _Static_assert /* C11 requires this #define.  */
 # endif
 #endif
-- 
2.27.0

Re: clang++ 11 compilation issues

[Bruno Haible]

Paul Eggert wrote:
> How about the attached, more-conservative patch instead? It limits 
> __cpp_static_assert to controlling the use of 'static_assert', and 
> causes C++ code to not use _Static_assert at all.

This one looks good. I agree that most C++ compilers have or will have
'static_assert', therefore using the fact that some clang versions support
'_Static_assert' also in C++ mode is redundant.

Bruno

Re: clang++ 11 compilation issues

[Alexandre Duret-Lutz]

Bruno Haible <bruno@clisp.org> writes:

> You are supposed to choose the warnings that are reasonable for your
> project. For some project of mine, I had to disable 20-40 warning options
> before I could get reasonable output.

That's exactly what I did :-) The set of warnings enabled in those
commands is the subset of warnings for which my project compiles
warning-free for that compiler on the different architectures I test.
Or at least it used to be warning free before I attempted to upgrade its
gnulib directory.

I have configure checks to detect which warning I can enable depending
on the compiler, and disable some warnings in version of GCC where they
are bogus.

I've done these extreme warnings on this project since 2003, but only on
my development builds, so I can detect and fix any new concerns brought
up by newer compiler versions.  Of course users builds are not done with
those options.

Obviously I target a much smaller and more modern set of systems than
gnulib, so I perfectly understand that you may not want to fix all
warnings.

>> > ../lib/gettext.h:234:22: error: zero as null pointer constant [-Werror,-Wzero-as-null-pointer-constant]
>> >   if (msg_ctxt_id != NULL)
>> >                      ^~~~
>
> This will not be changed. 

I have another related suggestion.

My code is not internationalized.  gettext.h got pulled in because some
modules like argp are internationalized, but ENABLE_NLS will never be
defined in my project.

So this above warning is about code that will never be used, but for which
the compiler is still wasting resources for parsing and analyzing.

Would it be acceptable to replace all these pgettext functions by dummy
macros when NLS is disabled?
-- 
Alexandre Duret-Lutz

different CFLAGS for gnulib code?

[Bruno Haible]

Hi,

Jeffrey Walton wrote:
> Perhaps it would be a good idea to filter-out the options that you
> don't want present for Gnulib.
> 
> If you are doing it during configure, then take the user's CFLAGS (or
> CXXFLAGS) and then:
> 
>        TCFLAGS=`echo $CFLAGS | sed -e 's/-Wall//g' -e 's/-Wextra//g'
> -e 's/-Werror//g'`
> 
> If you are doing it during make, then use a recipe like this for Gnulib sources:
> 
>     GL_CFLAGS := $(filter-out -Wall -Wextra -Werror% -Wunused
> -Wconversion -Wp%, $(CFLAGS))
>     ...
>     %.o:%.c:
>         $CC $(strip $CPPFLAGS $GL_CFLAGS -c) $<
> 
> That will put an end to these mailing list messages and bug reports.
> You get what you want, and users get what they want.
> 
> Otherwise, this is an exercise in insanity. Users keep doing the same
> thing, GNU keeps doing the same thing, but everyone expects a
> different outcome. Instead of practicing inanity, engineer a fix for
> the problem.

It is an interesting idea. Leaving the question aside how it is implemented
(through an AC_SUBSTed variable or what else), the main question is: Would
some GNU package maintainers want this?

I always thought that GNU package maintainers want their entire package to
be compiled with the same CFLAGS and CPPFLAGS. Would compiling the gnulib
part with options for fewer warnings be OK with you?

Paul, Pádraig, Jim, Paul, Akim, Simon, all: what's your opinion?

Bruno

Re: different CFLAGS for gnulib code?

[Alexandre Duret-Lutz]

Bruno Haible <bruno@clisp.org> writes:

> I always thought that GNU package maintainers want their entire package to
> be compiled with the same CFLAGS and CPPFLAGS. Would compiling the gnulib
> part with options for fewer warnings be OK with you?

Just note that none of the warnings and errors I complained about
recently occur when compiling gnulib, which I compile without any extra
warning flags anyway.  They occur when compiling my own C++ code that include
gnulib's headers.

-- 
Alexandre Duret-Lutz

Re: different CFLAGS for gnulib code?

[Paul Eggert]

On 1/15/21 12:55 AM, Bruno Haible wrote:
> Would compiling the gnulib
> part with options for fewer warnings be OK with you?

Not only is it OK, it's routine. Coreutils, Emacs, etc. do it.

Re: different CFLAGS for gnulib code?

[Simon Josefsson via Gnulib discussion list]

[-- Attachment #1: Type: text/plain, Size: 1858 bytes --]

fre 2021-01-15 klockan 09:55 +0100 skrev Bruno Haible:
> It is an interesting idea. Leaving the question aside how it is
> implemented
> (through an AC_SUBSTed variable or what else), the main question is:
> Would
> some GNU package maintainers want this?
> 
> I always thought that GNU package maintainers want their entire
> package to
> be compiled with the same CFLAGS and CPPFLAGS. Would compiling the
> gnulib
> part with options for fewer warnings be OK with you?
> 
> Paul, Pádraig, Jim, Paul, Akim, Simon, all: what's your opinion?

In general I think different code need different flags.  Some things I
consider a no-no in my own code is not something I would enforce on
gnulib code, and vice versa.

1) It is already possible to do this, just write a local Makefile.am
and use gnulib-tool --makefile-name=gnulib.mk and do local compile flag
changes.  I do this sometime to disable -Werror for gnulib but I want
the -Werror flag to be enabled for my own code.

2) Sometimes building gnulib with the package flags results in
improvements to gnulib that I wouldn't otherwise notice.  I recall lots
of fixes to gnulib originating this way.

3) Sometimes building gnulib with the package flag causes problems, in
the past this happened with -Werror and I've stopped using -Werror by
default (I think) because gnulib's code tends to be more fragile when
it comes to compiler-specific detailed warnings than some of my own
code.

4) I'd like separate --enable-silent-rule settings for gnulib and my
own code.  When developing from git, I really don't want to see all of
gnulib's output but I would like to see the project's own output.  This
is probably possible to achieve using a gnulib.mk approach.  Having
native support for --enable-silent-gnulib-rules or support for 'make
V=2' would be useful.

/Simon


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: different CFLAGS for gnulib code?

[Jim Meyering]

On Fri, Jan 15, 2021 at 12:55 AM Bruno Haible <bruno@clisp.org> wrote:
> Jeffrey Walton wrote:
> > Perhaps it would be a good idea to filter-out the options that you
> > don't want present for Gnulib.
...
> It is an interesting idea. Leaving the question aside how it is implemented
> (through an AC_SUBSTed variable or what else), the main question is: Would
> some GNU package maintainers want this?
>
> I always thought that GNU package maintainers want their entire package to
> be compiled with the same CFLAGS and CPPFLAGS. Would compiling the gnulib
> part with options for fewer warnings be OK with you?
>
> Paul, Pádraig, Jim, Paul, Akim, Simon, all: what's your opinion?

Yes, I do something like that in every package for which I make
releases, these days.
There is a separate clause in configure.ac where we arrange to remove
troublesome warning options from the global list to form the CFLAGS
options used for gnulib proper:
AC_SUBST([GNULIB_WARN_CFLAGS])

and sometimes also (e.g., in grep) where we do similar for gnulib tests:
  AC_SUBST([GNULIB_TEST_WARN_CFLAGS])

Re: different CFLAGS for gnulib code?

[Darshit Shah]

On 15.01.21 12:11, Paul Eggert wrote:
> On 1/15/21 12:55 AM, Bruno Haible wrote:
>> Would compiling the gnulib
>> part with options for fewer warnings be OK with you?
> 
> Not only is it OK, it's routine. Coreutils, Emacs, etc. do it.
> 

Wget does the same thing as well. I treat gnulib as another library,
libraries are not necessarily compiled with the same compiler options.

Re: different CFLAGS for gnulib code?

[Pádraig Brady]

On 15/01/2021 08:55, Bruno Haible wrote:
> I always thought that GNU package maintainers want their entire package to
> be compiled with the same CFLAGS and CPPFLAGS. Would compiling the gnulib
> part with options for fewer warnings be OK with you?
> 
> Paul, Pádraig, Jim, Paul, Akim, Simon, all: what's your opinion?

Re coreutils we already have different warnings,
specifically a subset of warnings are used with gnulib.

gnulib is necessarily more general/portable than a particular project,
and so could not be as generally stringent in its warnings I think.
Also a new project has more flexibility to enable a stricter set
from the start, than a mature code base like gnulib.

cheers,
Pádraig