Message ID | 20140318020544.GO1850@spoyarek.pnq.redhat.com |
---|---|
State | New |
Headers | show |
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 > >
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
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.
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
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.
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
-----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-----
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 --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