argp: Do not override GCC keywords with macros [BZ #16907]

glibc provides fallback definitions already.  It is not necessary to
suppress warnings for unknown attributes because GCC does this
automatically for system headers.

This commit does not sync with gnulib because gnulib has started to use
_GL_* macros in the header file, which are arguably in the gnulib
implementation space and not suitable for an installed glibc header
file.

2016-08-14  Florian Weimer  <fweimer@redhat.com>

	[BZ #16907]
	* argp/argp.h: Switch to __BEGIN_DECLS and __END_DECLS.
	(__THROW, __NTH, __attribute__, __restrict): Remove definitions.
	* argp/argp-fmtstream.h: Add __BEGIN_DECLS and __END_DECLS.
	(__attribute__): Remove definition.

Florian Weimer wrote:
> This commit does not sync with gnulib because gnulib has started to use
> _GL_* macros in the header file, which are arguably in the gnulib
> implementation space and not suitable for an installed glibc header
> file.

I'd rather keep the files in sync if possible. How about if we rename the 
affected macro from _GL_ATTRIBUTE_FORMAT to __attribute_format__ (or whatever 
other name you like that is in the glibc space? That way, Gnulib can stay in 
sync better.

Similarly, the files shared with Gnulib can define __BEGIN_DECLS and __END_DECLS 
when __BEGIN_DECL is not already defined.

On 08/15/2016 04:18 AM, Paul Eggert wrote:
> Florian Weimer wrote:
>> This commit does not sync with gnulib because gnulib has started to use
>> _GL_* macros in the header file, which are arguably in the gnulib
>> implementation space and not suitable for an installed glibc header
>> file.
>
> I'd rather keep the files in sync if possible. How about if we rename
> the affected macro from _GL_ATTRIBUTE_FORMAT to __attribute_format__ (or
> whatever other name you like that is in the glibc space? That way,
> Gnulib can stay in sync better.

Hmm.

I found this in the gnulib version:

#if !_LIBC || defined __USE_EXTERN_INLINES

# if !_LIBC
#  define __argp_usage argp_usage
#  define __argp_state_help argp_state_help
#  define __option_is_short _option_is_short
#  define __option_is_end _option_is_end
#ifndef _GL_INLINE_HEADER_BEGIN
  #error "Please include config.h first."
#endif
_GL_INLINE_HEADER_BEGIN

This will result in an #error if the installed header <argp.h> is used 
by applications.

So full synchronization is currently not possible anyway.

Florian

Florian Weimer wrote:

> #ifndef _GL_INLINE_HEADER_BEGIN
>  #error "Please include config.h first."
> #endif
>
> This will result in an #error if the installed header <argp.h> is used by
> applications.

Then let's remove those three lines. They are not essential for Gnulib, and 
staying in sync with glibc is more important than the extra error-checking they 
might provide for Gnulib users.

On 08/15/2016 09:06 AM, Paul Eggert wrote:
> Florian Weimer wrote:
>
>> #ifndef _GL_INLINE_HEADER_BEGIN
>>  #error "Please include config.h first."
>> #endif
>>
>> This will result in an #error if the installed header <argp.h> is used by
>> applications.
>
> Then let's remove those three lines. They are not essential for Gnulib,
> and staying in sync with glibc is more important than the extra
> error-checking they might provide for Gnulib users.

We also have to adjust the use of the macros _GL_INLINE_HEADER_BEGIN, 
_GL_INLINE_HEADER_END, and _GL_INLINE.  The gnulib version currently 
assumes that _LIBC is always defined for a glibc header, which is not 
true when the header is installed.

I would like to use the minimal patch as a start.  We can sync with 
gnulib once it has fixed the non-gnulib, !_LIBC case.

Florian

On 08/14/2016 03:01 PM, Florian Weimer wrote:
> glibc provides fallback definitions already.  It is not necessary to
> suppress warnings for unknown attributes because GCC does this
> automatically for system headers.

Does gcc do this for all known versions that we support compiling
applications with?

We support compiling applications with new glibc headers, but old
gcc, and we shouldn't break that, though we need not optimize for it.

https://sourceware.org/ml/libc-alpha/2015-05/msg00526.html

Cheers,
Carlos.

> This commit does not sync with gnulib because gnulib has started to use
> _GL_* macros in the header file, which are arguably in the gnulib
> implementation space and not suitable for an installed glibc header
> file.
> 
> 2016-08-14  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #16907]
> 	* argp/argp.h: Switch to __BEGIN_DECLS and __END_DECLS.
> 	(__THROW, __NTH, __attribute__, __restrict): Remove definitions.
> 	* argp/argp-fmtstream.h: Add __BEGIN_DECLS and __END_DECLS.
> 	(__attribute__): Remove definition.
> 
> diff --git a/argp/argp-fmtstream.h b/argp/argp-fmtstream.h
> index bdeaa54..e8c5797 100644
> --- a/argp/argp-fmtstream.h
> +++ b/argp/argp-fmtstream.h
> @@ -29,21 +29,6 @@
>  #include <string.h>
>  #include <unistd.h>
>  
> -#ifndef __attribute__
> -/* This feature is available in gcc versions 2.5 and later.  */
> -# if __GNUC__ < 2 || (__GNUC__ == 2 && __GNUC_MINOR__ < 5) || \
> -  defined __STRICT_ANSI__
> -#  define __attribute__(Spec) /* empty */
> -# endif
> -/* The __-protected variants of `format' and `printf' attributes
> -   are accepted by gcc versions 2.6.4 (effectively 2.7) and later.  */
> -# if __GNUC__ < 2 || (__GNUC__ == 2 && __GNUC_MINOR__ < 7) || \
> -  defined __STRICT_ANSI__
> -#  define __format__ format
> -#  define __printf__ printf
> -# endif
> -#endif
> -
>  #if defined (__GNU_LIBRARY__) && defined (HAVE_LINEWRAP_H)
>  /* line_wrap_stream is available, so use that.  */
>  #define ARGP_FMTSTREAM_USE_LINEWRAP
> @@ -111,6 +96,8 @@ struct argp_fmtstream
>  
>  typedef struct argp_fmtstream *argp_fmtstream_t;
>  
> +__BEGIN_DECLS
> +
>  /* Return an argp_fmtstream that outputs to STREAM, and which prefixes lines
>     written on it with LMARGIN spaces and limits them to RMARGIN columns
>     total.  If WMARGIN >= 0, words that extend past RMARGIN are wrapped by
> @@ -297,6 +284,8 @@ __argp_fmtstream_point (argp_fmtstream_t __fs)
>  
>  #endif /* __OPTIMIZE__ */
>  
> +__END_DECLS
> +
>  #endif /* ARGP_FMTSTREAM_USE_LINEWRAP */
>  
>  #endif /* argp-fmtstream.h */
> diff --git a/argp/argp.h b/argp/argp.h
> index e67bbef..7cb5a69 100644
> --- a/argp/argp.h
> +++ b/argp/argp.h
> @@ -28,48 +28,12 @@
>  #define __need_error_t
>  #include <errno.h>
>  
> -#ifndef __THROW
> -# define __THROW
> -#endif
> -#ifndef __NTH
> -# define __NTH(fct) fct __THROW
> -#endif
> -
> -#ifndef __attribute__
> -/* This feature is available in gcc versions 2.5 and later.  */
> -# if __GNUC__ < 2 || (__GNUC__ == 2 && __GNUC_MINOR__ < 5) || \
> -  defined __STRICT_ANSI__
> -#  define __attribute__(Spec) /* empty */
> -# endif
> -/* The __-protected variants of `format' and `printf' attributes
> -   are accepted by gcc versions 2.6.4 (effectively 2.7) and later.  */
> -# if __GNUC__ < 2 || (__GNUC__ == 2 && __GNUC_MINOR__ < 7) || \
> -  defined __STRICT_ANSI__
> -#  define __format__ format
> -#  define __printf__ printf
> -# endif
> -#endif
> -
> -/* GCC 2.95 and later have "__restrict"; C99 compilers have
> -   "restrict", and "configure" may have defined "restrict".  */
> -#ifndef __restrict
> -# if ! (2 < __GNUC__ || (2 == __GNUC__ && 95 <= __GNUC_MINOR__))
> -#  if defined restrict || 199901L <= __STDC_VERSION__
> -#   define __restrict restrict
> -#  else
> -#   define __restrict
> -#  endif
> -# endif
> -#endif
> -
>  #ifndef __error_t_defined
>  typedef int error_t;
>  # define __error_t_defined
>  #endif
>  
> -#ifdef  __cplusplus
> -extern "C" {
> -#endif
> +__BEGIN_DECLS
>  
>  /* A description of a particular option.  A pointer to an array of
>     these is passed in the OPTIONS field of an argp structure.  Each option
> @@ -590,8 +554,6 @@ __NTH (__option_is_end (const struct argp_option *__opt))
>  # endif
>  #endif /* Use extern inlines.  */
>  
> -#ifdef  __cplusplus
> -}
> -#endif
> +__END_DECLS
>  
>  #endif /* argp.h */
>

On 08/15/2016 09:50 PM, Carlos O'Donell wrote:
> On 08/14/2016 03:01 PM, Florian Weimer wrote:
>> glibc provides fallback definitions already.  It is not necessary to
>> suppress warnings for unknown attributes because GCC does this
>> automatically for system headers.
>
> Does gcc do this for all known versions that we support compiling
> applications with?
>
> We support compiling applications with new glibc headers, but old
> gcc, and we shouldn't break that, though we need not optimize for it.
>
> https://sourceware.org/ml/libc-alpha/2015-05/msg00526.html

With GCC 2.7.2.3, an unknown attribute is just a warning, not a hard 
error.  It is not suppressed in system headers, though.  <sys/types.h> 
already produces a warning:

/usr/include/bits/pthreadtypes.h:123: warning: unnamed struct/union that 
defines no instances

And no one has complained.

I can go back further if absolutely necessary.

Florian

On 08/14/2016 09:01 PM, Florian Weimer wrote:

> 2016-08-14  Florian Weimer  <fweimer@redhat.com>
>
> 	[BZ #16907]
> 	* argp/argp.h: Switch to __BEGIN_DECLS and __END_DECLS.
> 	(__THROW, __NTH, __attribute__, __restrict): Remove definitions.
> 	* argp/argp-fmtstream.h: Add __BEGIN_DECLS and __END_DECLS.
> 	(__attribute__): Remove definition.

I have committed this.

Thanks,
Florian