Re: [PATCH v2] Make __extern_always_inline usable on clang++ again

["Carlos O'Donell" <carlos at redhat dot com>]

On 09/16/2014 07:26 AM, Siddhesh Poyarekar wrote:
> On Tue, Sep 16, 2014 at 12:41:46PM +0200, Jakub Jelinek wrote:
>> s/fot/for/; why are you using a separate hunk for clang instead of using the
>> earlier one?
> 
> I thought it looked ugly when I wrote the entire conditional in one
> place but I notice now (since you pointed it out) that it actually
> makes things wrong.
> 
>>> +#if !defined __extern_always_inline && defined __clang__
>>> +# if defined __GNUC_STDC_INLINE__ || defined __GNUC_GNU_INLINE__
>>> +#  define __extern_inline extern __inline __attribute__ ((__gnu_inline__))
>>> +#  define __extern_always_inline \
>>> +  extern __always_inline __attribute__ ((__gnu_inline__))
>>> +# else
>>> +#  define __extern_inline extern __inline
>>> +#  define __extern_always_inline extern __always_inline
>>
>> This doesn't look right.  If you have clang that doesn't have gnu_inline
>> support, in C++ extern __inline definitely is not the right semantics.
>> You don't want to define the macros then.
> 
> Here's the updated patch, again tested with the same program.
> 
> Siddhesh
> 
> 	[BZ #17266]
> 	* misc/sys/cdefs.h: Define __extern_always_inline for clang
> 	4.2 and newer.
> 
> 
> diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
> index d8ee73c..6a789e7 100644
> --- a/misc/sys/cdefs.h
> +++ b/misc/sys/cdefs.h
> @@ -318,8 +318,14 @@
>  #endif
>  
>  /* GCC 4.3 and above with -std=c99 or -std=gnu99 implements ISO C99
> -   inline semantics, unless -fgnu89-inline is used.  */
> -#if !defined __cplusplus || __GNUC_PREREQ (4,3)
> +   inline semantics, unless -fgnu89-inline is used.
> +
> +   clang++ identifies itself as gcc-4.2, but has support for GNU inlining
> +   semantics, that can be checked for by using the __GNUC_STDC_INLINE_ and
> +   __GNUC_GNU_INLINE__ macro definitions.  */

OK, Good comments.

> +#if (!defined __cplusplus || __GNUC_PREREQ (4,3) \
> +     || (defined __clang__ && (defined __GNUC_STDC_INLINE__ \
> +			       || defined __GNUC_GNU_INLINE__)))
>  # if defined __GNUC_STDC_INLINE__ || defined __cplusplus
>  #  define __extern_inline extern __inline __attribute__ ((__gnu_inline__))
>  #  define __extern_always_inline \

The existing code says:
(1) If GCC 4.3 or later and __GNUC_STDC_INLINE__ then add __gnu_inline__.
(2) If GCC 4.3 or later and it's C++ then add __gnu_inline__.
(3) if it's not C++ and __GNUC_STDC_INLINE__ then add __gnu_inline__

The new code says:
(1) If GCC 4.3 or later and __GNUC_STDC_INLINE__ then add __gnu_inline__.
(2) If GCC 4.3 or later and it's C++ then add __gnu_inline__.
(3) If it's not C++ and __GNUC_STDC_INLINE__ then add __gnu_inline__.
(4) If Clang and __GNUC_STDC_INLINE__ then add __gnu_inline__.
	- Notice if we split the compilers up we might ahve folded this condition.
(5) If Clang and __GNUC_GNU_INLINE__ and C++ then add __gnu_inline__.

I omitted some of the redundant conditional sets.

This looks correct.

So OK.

My preference is still to have this conditional be distinct for
clang and instead set something like HAVE_FOO, similarly for gcc,
and then have a single #if HAVE_FOO to set or not set the definitions
to add or not __gnu_inline__.

This allows a cleaner representation and folding of the checks for
each compiler based on the compiler's intricacies.

Cheers,
Carlos.