diff mbox

Fix -Wundef warnins for __FP_FAST_FMA*

Message ID 20140318020544.GO1850@spoyarek.pnq.redhat.com
State New
Headers show

Commit Message

Siddhesh Poyarekar March 18, 2014, 2:05 a.m. UTC 
The macros are defined by the compiler, so we can only verify whether
they are defined or not.

I have made changes to the arm and tile bits as well, but I have not
tested them.  OK to commit?

Siddhesh

	* bits/mathdef.h: Use #ifdef instead of #if.
 	* sysdeps/arm/bits/mathdef.h [defined __USE_ISOC99 && defined
	_MATH_H && !defined _MATH_H_MATHDEF]: Likewise.
 	* sysdeps/tile/bits/mathdef.h [defined __USE_ISOC99 && defined
	_MATH_H && !defined _MATH_H_MATHDEF]: Likewise.
 	* sysdeps/x86/bits/mathdef.h [defined __USE_ISOC99 && defined
	_MATH_H && !defined _MATH_H_MATHDEF]: Likewise.


---
 bits/mathdef.h              | 6 +++---
 sysdeps/arm/bits/mathdef.h  | 6 +++---
 sysdeps/tile/bits/mathdef.h | 6 +++---
 sysdeps/x86/bits/mathdef.h  | 6 +++---
 4 files changed, 12 insertions(+), 12 deletions(-)

Comments

Carlos O'Donell March 18, 2014, 4:38 a.m. UTC | #1 
On 03/17/2014 10:05 PM, Siddhesh Poyarekar wrote:
> The macros are defined by the compiler, so we can only verify whether
> they are defined or not.

We can do more than that.
 
> I have made changes to the arm and tile bits as well, but I have not
> tested them.  OK to commit?

I'm including Roland in this discussion, because I'd like to hear
his input.

I feel like the correction solution to use a configure check to
determine if the compiler defines e.g. __FP_FAST_FMA, and then
explicitly define a compiler feature macro as 0 or 1.

This definitely makes the entire process robust. The configure
script knows what features the compiler has, defines macros for
them, and then you use #if in the code to catch any source of
errors from undefined macros caused by incorrectly configured
compilers etc.

As far as I know this is what we've always wanted e.g. the
configure script checking for compiler features and setting
compiler feature macros rather than testing compiler feature
macros directly.

This isn't always going to be what we want to do though since
for example some source is compiled with different flags that
change compiler defines and that would require a single source
file to include different headers based on compiler options
in order to support using only #if. However, in this case where
we have an explicit compiler feature to test, we should probably
check for that feature directly.

Comments?
 
> Siddhesh
> 
> 	* bits/mathdef.h: Use #ifdef instead of #if.
>  	* sysdeps/arm/bits/mathdef.h [defined __USE_ISOC99 && defined
> 	_MATH_H && !defined _MATH_H_MATHDEF]: Likewise.
>  	* sysdeps/tile/bits/mathdef.h [defined __USE_ISOC99 && defined
> 	_MATH_H && !defined _MATH_H_MATHDEF]: Likewise.
>  	* sysdeps/x86/bits/mathdef.h [defined __USE_ISOC99 && defined
> 	_MATH_H && !defined _MATH_H_MATHDEF]: Likewise.
> 
> 
> ---
>  bits/mathdef.h              | 6 +++---
>  sysdeps/arm/bits/mathdef.h  | 6 +++---
>  sysdeps/tile/bits/mathdef.h | 6 +++---
>  sysdeps/x86/bits/mathdef.h  | 6 +++---
>  4 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/bits/mathdef.h b/bits/mathdef.h
> index ca1f464..f27ecac 100644
> --- a/bits/mathdef.h
> +++ b/bits/mathdef.h
> @@ -35,15 +35,15 @@ typedef double double_t;	/* `double' expressions are evaluated as
>  
>  /* The GCC 4.6 compiler will define __FP_FAST_FMA{,F,L} if the fma{,f,l}
>     builtins are supported.  */
> -#if __FP_FAST_FMA
> +#ifdef __FP_FAST_FMA
>  # define FP_FAST_FMA 1
>  #endif
>  
> -#if __FP_FAST_FMAF
> +#ifdef __FP_FAST_FMAF
>  # define FP_FAST_FMAF 1
>  #endif
>  
> -#if __FP_FAST_FMAL
> +#ifdef __FP_FAST_FMAL
>  # define FP_FAST_FMAL 1
>  #endif
>  
> diff --git a/sysdeps/arm/bits/mathdef.h b/sysdeps/arm/bits/mathdef.h
> index be727e5..f309002 100644
> --- a/sysdeps/arm/bits/mathdef.h
> +++ b/sysdeps/arm/bits/mathdef.h
> @@ -34,15 +34,15 @@ typedef double double_t;	/* `double' expressions are evaluated as
>  
>  /* The GCC 4.6 compiler will define __FP_FAST_FMA{,F,L} if the fma{,f,l}
>     builtins are supported.  */
> -# if __FP_FAST_FMA
> +# ifdef __FP_FAST_FMA
>  #  define FP_FAST_FMA 1
>  # endif
>  
> -# if __FP_FAST_FMAF
> +# ifdef __FP_FAST_FMAF
>  #  define FP_FAST_FMAF 1
>  # endif
>  
> -# if __FP_FAST_FMAL
> +# ifdef __FP_FAST_FMAL
>  #  define FP_FAST_FMAL 1
>  # endif
>  
> diff --git a/sysdeps/tile/bits/mathdef.h b/sysdeps/tile/bits/mathdef.h
> index d043b4a..c26a2e7 100644
> --- a/sysdeps/tile/bits/mathdef.h
> +++ b/sysdeps/tile/bits/mathdef.h
> @@ -33,15 +33,15 @@ typedef double double_t;
>  
>  /* The GCC 4.6 compiler will define __FP_FAST_FMA{,F,L} if the fma{,f,l}
>     builtins are supported.  */
> -# if __FP_FAST_FMA
> +# ifdef __FP_FAST_FMA
>  #  define FP_FAST_FMA 1
>  # endif
>  
> -# if __FP_FAST_FMAF
> +# ifdef __FP_FAST_FMAF
>  #  define FP_FAST_FMAF 1
>  # endif
>  
> -# if __FP_FAST_FMAL
> +# ifdef __FP_FAST_FMAL
>  #  define FP_FAST_FMAL 1
>  # endif
>  
> diff --git a/sysdeps/x86/bits/mathdef.h b/sysdeps/x86/bits/mathdef.h
> index 07c2d66..fd9cf42 100644
> --- a/sysdeps/x86/bits/mathdef.h
> +++ b/sysdeps/x86/bits/mathdef.h
> @@ -44,15 +44,15 @@ typedef long double double_t;	/* `double' expressions are evaluated as
>  
>  /* The GCC 4.6 compiler will define __FP_FAST_FMA{,F,L} if the fma{,f,l}
>     builtins are supported.  */
> -# if __FP_FAST_FMA
> +# ifdef __FP_FAST_FMA
>  #  define FP_FAST_FMA 1
>  # endif
>  
> -# if __FP_FAST_FMAF
> +# ifdef __FP_FAST_FMAF
>  #  define FP_FAST_FMAF 1
>  # endif
>  
> -# if __FP_FAST_FMAL
> +# ifdef __FP_FAST_FMAL
>  #  define FP_FAST_FMAL 1
>  # endif
>  
>
Siddhesh Poyarekar March 18, 2014, 5:07 a.m. UTC | #2 
On Tue, Mar 18, 2014 at 12:38:37AM -0400, Carlos O'Donell wrote:
> I'm including Roland in this discussion, because I'd like to hear
> his input.
> 
> I feel like the correction solution to use a configure check to
> determine if the compiler defines e.g. __FP_FAST_FMA, and then
> explicitly define a compiler feature macro as 0 or 1.

bits/mathdef.h is installed, so using a configure check here would be
wrong.

Siddhesh
Carlos O'Donell March 18, 2014, 1:58 p.m. UTC | #3 
On 03/18/2014 01:07 AM, Siddhesh Poyarekar wrote:
> On Tue, Mar 18, 2014 at 12:38:37AM -0400, Carlos O'Donell wrote:
>> I'm including Roland in this discussion, because I'd like to hear
>> his input.
>>
>> I feel like the correction solution to use a configure check to
>> determine if the compiler defines e.g. __FP_FAST_FMA, and then
>> explicitly define a compiler feature macro as 0 or 1.
> 
> bits/mathdef.h is installed, so using a configure check here would be
> wrong.

In that case as an installed header it would appear that we don't have
much choice but to make these #ifdef. I guess the existing comments about
them being compiler defines is sufficient.

Cheers,
Carlos.
Roland McGrath March 18, 2014, 10:36 p.m. UTC | #4 
Indeed installed headers and compiler predefines are importantly different
cases than most of what we want to clean up in this regard.  Still, I think
we should be aiming for the same goal: minimize the possibility of typo
bugs not detected by -Wundef.

The context of installed headers just means that we have to take care with
the name space issues and the wide variety of older (and non-GNU) compilers
and their option combinations that might be in use, and beware of how users
might mistakenly misuse any internal macros we expose.

The context of predefines usually means that the base thing we're looking
at provided by the compiler has to be tested with #ifdef.  (For any future
predefines, we should work with compiler folks to suggest that new things
follow the #if-friendly pattern of macros always defined to either 0 or 1
rather than the typo-prone pattern of sometimes-defined macros.)  But that
doesn't mean there isn't cleanup and typo-proofing we should be doing.  We
should make sure that each predefine is tested with #ifdef in exactly one
place.  That place can then define internal macros to 0 or 1 so they can be
used elsewhere in our headers with #if and -Wundef.  The one place might be
simply at the top of an installed header (if the need to test it is solely
within that file or others that already must #include it).  It might be in
a new topical helper header (e.g. bits/foobar-options.h) if that seems to
make the most sense.  It might be in some new consolidated place for
handling many such macros (e.g. bits/predefines.h), or perhaps directly in
features.h).  I think we'll want to see all the variety of uses of such
macros in our installed headers before we decide.  (That is, audit the
macro use more generally, not just the things popping out with -Wundef.)

In short, these are the difficult cases.  I'd kind of figured we'd knock
out all the cases in private code before tackling the nontrivial installed
header cases.


Thanks,
Roland
Carlos O'Donell March 18, 2014, 11:30 p.m. UTC | #5 
On 03/18/2014 06:36 PM, Roland McGrath wrote:
> Indeed installed headers and compiler predefines are importantly different
> cases than most of what we want to clean up in this regard.  Still, I think
> we should be aiming for the same goal: minimize the possibility of typo
> bugs not detected by -Wundef.
> 
> The context of installed headers just means that we have to take care with
> the name space issues and the wide variety of older (and non-GNU) compilers
> and their option combinations that might be in use, and beware of how users
> might mistakenly misuse any internal macros we expose.
> 
> The context of predefines usually means that the base thing we're looking
> at provided by the compiler has to be tested with #ifdef.  (For any future
> predefines, we should work with compiler folks to suggest that new things
> follow the #if-friendly pattern of macros always defined to either 0 or 1
> rather than the typo-prone pattern of sometimes-defined macros.)  But that
> doesn't mean there isn't cleanup and typo-proofing we should be doing.  We
> should make sure that each predefine is tested with #ifdef in exactly one
> place.  That place can then define internal macros to 0 or 1 so they can be
> used elsewhere in our headers with #if and -Wundef.  The one place might be
> simply at the top of an installed header (if the need to test it is solely
> within that file or others that already must #include it).  It might be in
> a new topical helper header (e.g. bits/foobar-options.h) if that seems to
> make the most sense.  It might be in some new consolidated place for
> handling many such macros (e.g. bits/predefines.h), or perhaps directly in
> features.h).  I think we'll want to see all the variety of uses of such
> macros in our installed headers before we decide.  (That is, audit the
> macro use more generally, not just the things popping out with -Wundef.)

That makes sense.

> In short, these are the difficult cases.  I'd kind of figured we'd knock
> out all the cases in private code before tackling the nontrivial installed
> header cases.

I have no preference, and I do not wish to impose any on those people wishing
to assist with this work. I think we can do them in almost any order, and still
win.

Cheers,
Carlos.
Siddhesh Poyarekar March 19, 2014, 2:05 a.m. UTC | #6 
On Tue, Mar 18, 2014 at 03:36:18PM -0700, Roland McGrath wrote:
> In short, these are the difficult cases.  I'd kind of figured we'd knock
> out all the cases in private code before tackling the nontrivial installed
> header cases.

This case fixes about 2100 warnings, which is why it came up first on
my list.

Siddhesh
Carlos O'Donell March 19, 2014, 4:16 a.m. UTC | #7 
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/18/2014 10:05 PM, Siddhesh Poyarekar wrote:
> On Tue, Mar 18, 2014 at 03:36:18PM -0700, Roland McGrath wrote:
>> In short, these are the difficult cases.  I'd kind of figured we'd knock
>> out all the cases in private code before tackling the nontrivial installed
>> header cases.
> 
> This case fixes about 2100 warnings, which is why it came up first on
> my list.

Let's check this in then. As Roland suggests the only thing to avoid
is the use of this constant *again* in an #ifdef.

Cheers,
Carlos.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJTKRoaAAoJECXvCkNsKkr/aysIAInhEW/QSfWSZTGziltQJrlM
E6MnF2BIzWFINsuyUt1D2lV39r3okroNm6D1vpXqqPalc8wGd+kDOWIr7Axv7Pdx
vLM1WdNoo/SA6BaJiyzETn3ubucJWmXLvQ/AG8Zh9j0qwZH7sz6T5FsMUaLtaU6R
nUUw5uDvIQwKHdnMQKlmrLjtLWgYu3KLWSLCvJCQ9ZdselrWf2B7MB3PK5zUBjrb
pg4MQ8y4UxEUEmXt2gnWf7px4nA7JdYihxg2PGkptikfXCiJvP57TlL4MbkgIldn
iUEZ43jAY/GzNoJhKTdKfCnhfH3PfTp5b5ObN9Z41h54kkcYCCDqoqMcDbthiew=
=hUhP
-----END PGP SIGNATURE-----
Mike Frysinger March 19, 2014, 5:42 a.m. UTC | #8 
On Tue 18 Mar 2014 15:36:18 Roland McGrath wrote:
> The context of predefines usually means that the base thing we're looking
> at provided by the compiler has to be tested with #ifdef.  (For any future
> predefines, we should work with compiler folks to suggest that new things
> follow the #if-friendly pattern of macros always defined to either 0 or 1
> rather than the typo-prone pattern of sometimes-defined macros.)

i like the sentiment, but i'm not sure about the practicality.  how do you go 
about deciding whether a macro should always be defined ?  the set of all 
possible defines gcc might create depending on configuration/target settings ?  
seems like you'd easily generate a crap ton of noisy defines and blow past the 
point of usefulness ...
-mike
diff mbox

Patch

diff --git a/bits/mathdef.h b/bits/mathdef.h
index ca1f464..f27ecac 100644
--- a/bits/mathdef.h
+++ b/bits/mathdef.h
@@ -35,15 +35,15 @@  typedef double double_t;	/* `double' expressions are evaluated as
 
 /* The GCC 4.6 compiler will define __FP_FAST_FMA{,F,L} if the fma{,f,l}
    builtins are supported.  */
-#if __FP_FAST_FMA
+#ifdef __FP_FAST_FMA
 # define FP_FAST_FMA 1
 #endif
 
-#if __FP_FAST_FMAF
+#ifdef __FP_FAST_FMAF
 # define FP_FAST_FMAF 1
 #endif
 
-#if __FP_FAST_FMAL
+#ifdef __FP_FAST_FMAL
 # define FP_FAST_FMAL 1
 #endif
 
diff --git a/sysdeps/arm/bits/mathdef.h b/sysdeps/arm/bits/mathdef.h
index be727e5..f309002 100644
--- a/sysdeps/arm/bits/mathdef.h
+++ b/sysdeps/arm/bits/mathdef.h
@@ -34,15 +34,15 @@  typedef double double_t;	/* `double' expressions are evaluated as
 
 /* The GCC 4.6 compiler will define __FP_FAST_FMA{,F,L} if the fma{,f,l}
    builtins are supported.  */
-# if __FP_FAST_FMA
+# ifdef __FP_FAST_FMA
 #  define FP_FAST_FMA 1
 # endif
 
-# if __FP_FAST_FMAF
+# ifdef __FP_FAST_FMAF
 #  define FP_FAST_FMAF 1
 # endif
 
-# if __FP_FAST_FMAL
+# ifdef __FP_FAST_FMAL
 #  define FP_FAST_FMAL 1
 # endif
 
diff --git a/sysdeps/tile/bits/mathdef.h b/sysdeps/tile/bits/mathdef.h
index d043b4a..c26a2e7 100644
--- a/sysdeps/tile/bits/mathdef.h
+++ b/sysdeps/tile/bits/mathdef.h
@@ -33,15 +33,15 @@  typedef double double_t;
 
 /* The GCC 4.6 compiler will define __FP_FAST_FMA{,F,L} if the fma{,f,l}
    builtins are supported.  */
-# if __FP_FAST_FMA
+# ifdef __FP_FAST_FMA
 #  define FP_FAST_FMA 1
 # endif
 
-# if __FP_FAST_FMAF
+# ifdef __FP_FAST_FMAF
 #  define FP_FAST_FMAF 1
 # endif
 
-# if __FP_FAST_FMAL
+# ifdef __FP_FAST_FMAL
 #  define FP_FAST_FMAL 1
 # endif
 
diff --git a/sysdeps/x86/bits/mathdef.h b/sysdeps/x86/bits/mathdef.h
index 07c2d66..fd9cf42 100644
--- a/sysdeps/x86/bits/mathdef.h
+++ b/sysdeps/x86/bits/mathdef.h
@@ -44,15 +44,15 @@  typedef long double double_t;	/* `double' expressions are evaluated as
 
 /* The GCC 4.6 compiler will define __FP_FAST_FMA{,F,L} if the fma{,f,l}
    builtins are supported.  */
-# if __FP_FAST_FMA
+# ifdef __FP_FAST_FMA
 #  define FP_FAST_FMA 1
 # endif
 
-# if __FP_FAST_FMAF
+# ifdef __FP_FAST_FMAF
 #  define FP_FAST_FMAF 1
 # endif
 
-# if __FP_FAST_FMAL
+# ifdef __FP_FAST_FMAL
 #  define FP_FAST_FMAL 1
 # endif