Date: Wed, 7 Mar 2012 14:29:28 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Dimitry Andric <dim@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r232620 - in head/include: . xlocale Message-ID: <20120307120126.K969@besplex.bde.org> In-Reply-To: <201203062015.q26KFNJA055743@svn.freebsd.org> References: <201203062015.q26KFNJA055743@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 6 Mar 2012, Dimitry Andric wrote:
> Log:
> After r232498, programs built with -ansi or -std=c89 including <ctype.h>
> would not compile anymore, due to plain 'inline' keywords. Fix this by
> using __inline instead.
>
> Reported by: Jia-Shiun Li <jiashiun@gmail.com>
> Discussed with: theraven
Any chance of also fixing the style bugs? Old ctype was one of the
most carefully written headers, so it was almost free of style bugs.
> Modified: head/include/runetype.h
> ==============================================================================
> --- head/include/runetype.h Tue Mar 6 20:01:25 2012 (r232619)
> +++ head/include/runetype.h Tue Mar 6 20:15:23 2012 (r232620)
> @@ -90,7 +90,7 @@ extern const _RuneLocale *_CurrentRuneLo
> extern const _RuneLocale *__getCurrentRuneLocale(void);
Old ctype had no style bugs like declaring prototypes as extern.
Old ctype carefully indented prototypes.
> #else
> extern _Thread_local const _RuneLocale *_ThreadRuneLocale;
> -static inline const _RuneLocale *__getCurrentRuneLocale(void)
> +static __inline const _RuneLocale *__getCurrentRuneLocale(void)
> {
>
> if (_ThreadRuneLocale)
>
> Modified: head/include/xlocale/_ctype.h
> ==============================================================================
> --- head/include/xlocale/_ctype.h Tue Mar 6 20:01:25 2012 (r232619)
> +++ head/include/xlocale/_ctype.h Tue Mar 6 20:15:23 2012 (r232620)
> @@ -55,11 +55,11 @@ _RuneLocale *__runes_for_locale(locale_t
> #ifndef _XLOCALE_INLINE
> #if __GNUC__ && !__GNUC_STDC_INLINE__
> /* GNU89 inline has nonstandard semantics. */
> -#define _XLOCALE_INLINE extern inline
> +#define _XLOCALE_INLINE extern __inline
Old ctype always used a tab after #define.
Old ctype even used a tab after macro names.
> #else
> /* Hack to work around people who define inline away */
Old ctype mostly terminated its comments.
This comment now doesn't match the code. The code is now supposed to
use __inline, so it isn't affected by defining away inline.
> #ifdef inline
This part of the code was nonsense and hasn't really changed. The
code should be using __inline, so it shouldn't be affected by any
definition of inline. <sys/cdefs.h> defines both inline and __inline
to support old compilers. FreeBSD headers should not have their own
ifdefs for this.
> -#define _XLOCALE_INLINE __inline static
> +#define _XLOCALE_INLINE static __inline
This change is just a partial style fix. __inline was already used here,
and still is, but in a different order.
2 style bugs remain in this line (same 2 as above).
> #else
> /* Define with C++ / C99 compatible semantics */
> #define _XLOCALE_INLINE inline
This still doesn't use __inline, so it is still broken in most cases where
it is reached at all, if any. `inline' not being defined has very little
to do with whether it actually works here.
There are some complications for old gcc extern inline vs C99 extern
inline, but otherwise this code should just use __inline as defined
in <sys/cdefs.h> and not add its own ifdef tangle to the one there.
Or better yet, use only static __inline like old ctype does. I once
hoped to replace the static inlines in ctype.h by extern inline ones,
but that was before changing the semantics of extern inline made it
evern harder to use. Using it in libm mainly gives unportability to
C90.
Repeating the above, with the ifdefs untangled a bit:
> Modified: head/include/xlocale/_ctype.h
> ==============================================================================
> --- head/include/xlocale/_ctype.h Tue Mar 6 20:01:25 2012 (r232619)
> +++ head/include/xlocale/_ctype.h Tue Mar 6 20:15:23 2012 (r232620)
> @@ -55,11 +55,11 @@ _RuneLocale *__runes_for_locale(locale_t
> #ifndef _XLOCALE_INLINE
> #if __GNUC__ && !__GNUC_STDC_INLINE__
The first part of this should be written as defined(__GNUC__), to support
broken compilers that warn about an undefined identifier being used in an
ifdef.
Unfortunately, the simpler test 'ifdef __GNUC_GNU_INLINE__' seems to be
unusable, since old versions of gcc that only have gnu inline also don't
have a feature test for it.
> /* GNU89 inline has nonstandard semantics. */
> -#define _XLOCALE_INLINE extern inline
> +#define _XLOCALE_INLINE extern __inline
I'm not sure if this actually works. Old gnu extern inline is quite
different from C99 plain inline.
Anyway, the above seems to fix the main bug. Old version of gcc have
both inline and __inline, but -ansi or -std=c89 kills both. Except,
very old versions of gcc (gcc-1?) have neither. These are partially
handled by <sys/cdefs.h> killing __inline. <sys/cdefs.h> normally
doesn't kill inline; it takes the namespace pollution NO_ANSI_KEYWORDS
to do that; the semantics of this are even subtler. The fix seems
to be only partial, since "extern inline" in any spelling simply
doesn't work for compilers that don't support inline in any form.
Here we have slightly wrong ifdefs that lead gcc-1 being misclassified
and then mishandled in this clause of the ifdef tangle. The non-working
spelling is now "extern" instead of "extern inline". The "extern" has
been reduced to a style bug, since without a non-null "inline", it has
no effect, but it needs to have an effect to work.
> #else
Now we have classified the compiler as either non-gcc, or gcc with
STDC extern inline.
> /* Hack to work around people who define inline away */
> #ifdef inline
> -#define _XLOCALE_INLINE __inline static
> +#define _XLOCALE_INLINE static __inline
> #else
> /* Define with C++ / C99 compatible semantics */
> #define _XLOCALE_INLINE inline
It is now clearer that the inline ifdef has almost no effect. `inline'
should never be defined. <sys/cdefs.h> only defines it under weird
obsolete conditions, and the application shouldn't define it. But if
it might be defined, then we get the following behaviour from its ifdef:
- if it is defined, then we use static __inline. If this actually works,
then it is what we should use, without ifdefs, in all cases.
- if it is not defined, then we assume C99 extern inline semantics (with
implicit extern (IIRC, the extern is still needed, but is placed
weirdly, not in the header file but in the C file; thus it is not
part of the public interface and needs slightly different ifdefs).
This assumption is invalid, since we haven't done a full classification
of the compiler. We only know that the compiler supports C99 extern
semantics if it is gcc. This leaves the following broken cases:
- any C90 compiler that is not gcc
- any K&R compiler
In both of the cases, the compiler won't support inline in any form, so
the only chance for working is to use "static __inline" where
<sys/cdefs.h> kills __inline, or to not have any inline functions in
headers. Old <ctype.h> did the latter, and still works with sources
installed on 19 Feb 2012. Now it declares bazillions of inline
functions. Most kernel headers do the former.
Differences between the results of cc -E and cc -E
-I<up-to-date-src/include> on the file
#undef __GNUC__
#include <ctype.h>
% diff -c2 a~ a
% *** a~ Wed Mar 7 02:30:02 2012
% --- a Wed Mar 7 02:30:11 2012
% ***************
% *** 4,11 ****
% # 1 "a.c"
%
% ! # 1 "/usr/include/ctype.h" 1 3 4
% ! # 44 "/usr/include/ctype.h" 3 4
% # 1 "/usr/include/sys/cdefs.h" 1 3 4
% ! # 45 "/usr/include/ctype.h" 2 3 4
% # 1 "/usr/include/sys/_types.h" 1 3 4
% # 33 "/usr/include/sys/_types.h" 3 4
% --- 4,11 ----
% # 1 "a.c"
%
% ! # 1 "include/ctype.h" 1
% ! # 44 "include/ctype.h"
% # 1 "/usr/include/sys/cdefs.h" 1 3 4
% ! # 45 "include/ctype.h" 2
% # 1 "/usr/include/sys/_types.h" 1 3 4
% # 33 "/usr/include/sys/_types.h" 3 4
% ***************
% *** 136,142 ****
% __int64_t _mbstateL;
% } __mbstate_t;
% ! # 46 "/usr/include/ctype.h" 2 3 4
% ! # 1 "/usr/include/_ctype.h" 1 3 4
% ! # 70 "/usr/include/_ctype.h" 3 4
%
% unsigned long ___runetype(__ct_rune_t) ;
% --- 136,142 ----
% __int64_t _mbstateL;
% } __mbstate_t;
% ! # 46 "include/ctype.h" 2
% ! # 1 "include/_ctype.h" 1
% ! # 70 "include/_ctype.h"
%
% unsigned long ___runetype(__ct_rune_t) ;
% ***************
% *** 144,150 ****
% __ct_rune_t ___toupper(__ct_rune_t) ;
%
% ! # 86 "/usr/include/_ctype.h" 3 4
% extern int __mb_sb_limit;
% ! # 172 "/usr/include/_ctype.h" 3 4
%
% int __maskrune(__ct_rune_t, unsigned long);
% --- 144,150 ----
% __ct_rune_t ___toupper(__ct_rune_t) ;
%
% ! # 86 "include/_ctype.h"
% extern int __mb_sb_limit;
% ! # 172 "include/_ctype.h"
%
% int __maskrune(__ct_rune_t, unsigned long);
% ***************
% *** 159,163 ****
% int __wcwidth(__ct_rune_t);
%
% ! # 47 "/usr/include/ctype.h" 2 3 4
%
%
% --- 159,163 ----
% int __wcwidth(__ct_rune_t);
%
% ! # 47 "include/ctype.h" 2
%
%
% ***************
% *** 195,197 ****
% --- 195,274 ----
%
%
% +
% + # 1 "include/xlocale/_ctype.h" 1
% + # 44 "include/xlocale/_ctype.h"
This include is about the only change in ctype.h.
% + typedef struct _xlocale *locale_t;
xlocale also gives massive namespace pollution. locale_t isn't in
C90. Compiling with -std=c89 -pedantic doesn't change the output at all.
% +
% +
% +
% +
% + unsigned long ___runetype_l(__ct_rune_t, locale_t) ;
The strange formatting (space before semicolon) is due to a __pure being
there. __pure is correctly ifdefed, so my #undef of __GNUC__ kills it
% + __ct_rune_t ___tolower_l(__ct_rune_t, locale_t) ;
% + __ct_rune_t ___toupper_l(__ct_rune_t, locale_t) ;
% + _RuneLocale *__runes_for_locale(locale_t, int*);
The misformatting of "int*" is in the source code.
The ifdefs are broken in other ways. _RuneLocale is not declared here.
Without the #undef of __GNUC__, it is declared, and the preprocessed
output is 132 lines longer and all compile, and the extra lines seem
to be mostly correct too, since they are careful about namespaces and
don't declare any new APIs.
% + # 91 "include/xlocale/_ctype.h"
% + static int
__inline was corrctly killed here.
% + __sbmaskrune_l(__ct_rune_t _c, unsigned long _f, locale_t locale)
Namespace pollution (parameter name `locale' in the application namesspace.
The other arg names are from old ctype code which is missing such bugs.
% + {
% + int mb_sb_limit;
More namespace pollution.
% + _RuneLocale *runes = __runes_for_locale(locale, &mb_sb_limit);
More namespace pollution.
Style bugs: initialization in declaration; no blank line after declarations).
% + return (_c < 0 || _c >= mb_sb_limit) ? 0 :
% + runes->__runetype[_c] & _f;
Style bugs: unnecessary line splitting; weird continuation indent of 7
spaces (4 would be KNF and 1 tab would be gnu).
% + }
% +
% + static int
% + __sbistype_l(__ct_rune_t _c, unsigned long _f, locale_t locale)
% + {
% + return (!!__sbmaskrune_l(_c, _f, locale));
% + }
Similarly.
% +
% +
% +
% +
% +
% +
% +
% + inline int isalnum_l(int c, locale_t l); inline int isalnum_l(int c, locale_t l) { return __sbistype_l(c, 0x00000100L|0x00000400L, l); }
Now since we misclassified the compiler and don't use __inline, we have
syntax errors from `inline'.
We also have namespace pollution in almost every indentifier:
- isalnum_l, locale_t: not in C89
- c, l: in application namespace
We also have a redundant forward declaration. This seems to be technically
necessary in some cases, to avoid warnings from -Wmissing-prototypes. But
surely any such warnings are bugs for inline functions, and the compilers
that don't support inline functions also don't support such warnings?
In the source code, the 'inline' part is spelled _XLOCALE_INLINE, and
it should reduce to the missing-prototype case, except I faked the compiler
not being gcc and with a real non-gcc there would be no warning.
The style bugs of hard explicit long constants and no space around the
binary operator '|' seem to be in old ctype code.
% + inline int isalpha_l(int c, locale_t l); inline int isalpha_l(int c, locale_t l) { return __sbistype_l(c, 0x00000100L, l); }
% + inline int isblank_l(int c, locale_t l); inline int isblank_l(int c, locale_t l) { return __sbistype_l(c, 0x00020000L, l); }
% + inline int iscntrl_l(int c, locale_t l); inline int iscntrl_l(int c, locale_t l) { return __sbistype_l(c, 0x00000200L, l); }
% + inline int isdigit_l(int c, locale_t l); inline int isdigit_l(int c, locale_t l) { return __sbistype_l(c, 0x00000400L, l); }
% + inline int isgraph_l(int c, locale_t l); inline int isgraph_l(int c, locale_t l) { return __sbistype_l(c, 0x00000800L, l); }
% + inline int ishexnumber_l(int c, locale_t l); inline int ishexnumber_l(int c, locale_t l) { return __sbistype_l(c, 0x00010000L, l); }
% + inline int isideogram_l(int c, locale_t l); inline int isideogram_l(int c, locale_t l) { return __sbistype_l(c, 0x00080000L, l); }
% + inline int islower_l(int c, locale_t l); inline int islower_l(int c, locale_t l) { return __sbistype_l(c, 0x00001000L, l); }
% + inline int isnumber_l(int c, locale_t l); inline int isnumber_l(int c, locale_t l) { return __sbistype_l(c, 0x00000400L, l); }
% + inline int isphonogram_l(int c, locale_t l); inline int isphonogram_l(int c, locale_t l) { return __sbistype_l(c, 0x00200000L, l); }
% + inline int isprint_l(int c, locale_t l); inline int isprint_l(int c, locale_t l) { return __sbistype_l(c, 0x00040000L, l); }
% + inline int ispunct_l(int c, locale_t l); inline int ispunct_l(int c, locale_t l) { return __sbistype_l(c, 0x00002000L, l); }
% + inline int isrune_l(int c, locale_t l); inline int isrune_l(int c, locale_t l) { return __sbistype_l(c, 0xFFFFFF00L, l); }
% + inline int isspace_l(int c, locale_t l); inline int isspace_l(int c, locale_t l) { return __sbistype_l(c, 0x00004000L, l); }
% + inline int isspecial_l(int c, locale_t l); inline int isspecial_l(int c, locale_t l) { return __sbistype_l(c, 0x00100000L, l); }
% + inline int isupper_l(int c, locale_t l); inline int isupper_l(int c, locale_t l) { return __sbistype_l(c, 0x00008000L, l); }
% + inline int isxdigit_l(int c, locale_t l); inline int isxdigit_l(int c, locale_t l) { return __sbistype_l(c, 0x00010000L, l); }
These bugs are easy to fix since they are all generated by 1 macro.
% + # 170 "include/xlocale/_ctype.h"
% + inline int digittoint_l(int, locale_t);
% + inline int tolower_l(int, locale_t);
% + inline int toupper_l(int, locale_t);
% +
% + inline int digittoint_l(int __c, locale_t __l)
% + { return __sbmaskrune_l((__c), 0xFF, __l); }
Mounds of style bugs here.
% +
% + inline int tolower_l(int __c, locale_t __l)
% + {
% + int __limit;
% + _RuneLocale *__runes = __runes_for_locale(__l, &__limit);
% + return (__c < 0 || __c >= __limit) ? __c :
% + __runes->__maplower[__c];
% + }
% + inline int toupper_l(int __c, locale_t __l)
% + {
% + int __limit;
% + _RuneLocale *__runes = __runes_for_locale(__l, &__limit);
% + return (__c < 0 || __c >= __limit) ? __c :
% + __runes->__mapupper[__c];
% + }
As above, but is more careful about namespaces. This uses ugly
unecessary double underscores instead of the necessary single
underscores and the broken no underscores. Now the only namespace
pollution is the non-C90 interfaces.
% + # 84 "include/ctype.h" 2
% +
% +
% # 2 "a.c" 2
Next I tried defining inline as 'glorp'. This worked as correctly as
possible -- it changed all the plain inlines to `static' (according
to the ifdefs that avoid using inline if someone is hacking on it) and
didn't cause any syntax errors. Of course the glorp would cause syntax
errors in anything that uses plain inline.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120307120126.K969>