Re: [PATCH] libstdc++: implement locale support for AIX

["CHIGOT, CLEMENT" <clement.chigot@atos.net>]

Hi Rainer

> > 3) POSIX 2017 and non-POSIX functions
> > Many of the *_l functions being used in GNU or dragonfly models aren't
> > POSIX 2008, but mainly POSIX 2017 or like strtof_l not POSIX at all.
> > However, there are really useful in the code, thus I've made a double
> > implementation based on "#ifdef HAVE_". Is it ok for you ? It's not really
> > POSIX 2008 but more POSIX 2008 with 2017 compatibility.
> > For the configure, I didn't find any better way to check each syscall, as
> > they all depend on different includes. Tell me if you have a better idea.
>
> First a general observation: there are two groups of functions you're
> testing for:
>
> * Pure BSD additions, not available in either POSIX.1, ISO C, or glibc:
>
>  localeconv_l
>  mbstowcs_l
>  strtod_l
>  strtof_l
>  strtold_l
>  wcsftime_l
>
> * Part of XPG7:
>
>   iswctype_l
>   strcoll_l
>   strftime_l
>   strxfrm_l
>   towlower_l
>   towupper_l
>   wcscoll_l
>   wcsxfrm_l
>   wctype_l
>
> My suggestion would be not to have configure tests _GLIBCXX_HAVE_<FUNC>
> for any of the second group at all: this is ieee_1003.1-2008, after all,
> so if some OS selects that clocale variant, it better implement all of
> those.  If really need be, one could a configure check for those and
> error out if any is missing.  This makes the code way more readable than
> trying to handle some hypothetical partial implementation.

In this case, it would be better to call it ieee_1003.1-2017 but I agree
it would be better to avoid all these #ifdef.
Some are still needed as for example only the last version of AIX have
strftime_l.

> As for the BSD group, I suggest to have one representative configure
>  test (for localeconv_l perhaps) and then use an appropriate name for the
> group as a whole.  Again, this will most likely be an all-or-nothing
> thing.

I'm not sure this is really all-or-nothing for these. Maybe strtof_l and cie
can be grouped by. But the 3 others are really different. Linux have wcsftime_l
but not the others. AIX avec none. BSD have all.

> Besides, your configure tests are way too complicated: just use
> AC_CHECK_FUNCS doing a link test and be done with it.

Sadly, you can't pass includes to AC_CHECK_FUNCS. That's why I had to do
that. I've made a first version with AC_CHECK_DECLS which allows extra
headers, but it didn't work too. I might know why though.

> In a similar vein, configure.ac already has
> AC_CHECK_HEADERS([xlocale.h]).  Rather than hardcoding the existance of
> the header based on the configure triple, just use the existing
> HAVE_XLOCALE_H.  This ways, things will simply fall into place for
> e.g. NetBSD, OpenBSD and possibly others.

Right, I'll make the change. Thanks !

> > 4) ctype_configure_char.cc
> > I've some troubles knowing what is supposed to be implemented on this file.
> > I don't really understand the part with setlocale which appears in many
> > os. When I'm adding it, some tests start failing, some start working...
> > Moreover, on Linux, if I understand correctly, there is some optimizations
> > based on classic_table(), _M_toupper and _M_tolower. Could you confirm
> > that it's only useful on Linux ?
>
> I don't know myself.  However, when trying the first version of your
> patch (augmented to compile on Solaris), the corresponding change to the
> solaris file made no difference in test results.

I might have found the correct code since yesterday's mail. The problem seems
to come from _M_c_locale_ctype initialization. With locale support, it must be
_S_clone_c_locale(__cloc), without it, it must be the default locale which ends up
being "C". I might push a newer patch this afternoon, with the correct code.

> > Feel free to try in on other OS. But I've made modifications only for AIX and
> > Linux, as I can test the other ones.
>
> While reading through the patch, I saw that in two places you still use
> __DragonFly__ || __FreeBSD__ tests.  For one, it's hard to tell what
> feature they are really about, besides they will require fiddling with
> e.g. for other BSDs.  Please use a descriptive macro which says which
> difference this is about.

Right, because I don't know how to handle them (and I've forgotten to ask
for it...).
The first is for typedef __c_locale. It seems to be int* instead of locale_t.
Could you confirm that this is wanted and mandatory ?
The second is in about some functions in ctype_members.cc which are
defined in config/os/../ctype_inlines.h for FreeBSD and Dragonfly. Someone
has to confirm that it can be merged with the new code, or if this is mandatory.

> That said, I gave the new patch a try on Solaris 11.4.  To get it to
> compile, I had to apply two changes that I'd mentioned (without an actual
> patch) when commenting on the first patch:
>
> * The C99 fields of struct lconv need _LCONV_C99 to be visible for
>   C++11.
>
>  * Some ctype macros need __bitmapsize = 15, as the generic clocale
>   implementation uses.

If I'm not mistaking, POSIX is only defining 11 bit for ctype. If we want
some optimizations we can have a define of bitmasksize or we can simply
fill the whole mask by setting bitmasksize=15 as in generic.
I don't know what's best.

> Compared to the augmented first patch, there are a few differences: a
> couple of failures went away and I've now
>
> +XPASS: 22_locale/ctype/is/wchar_t/2.cc execution test

This is normal, because I need a way to check for which locale model
is being used.

Thanks,
Clément