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

[Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE>]

[-- Attachment #1: Type: text/plain, Size: 4175 bytes --]

Hi Clement,

> Here is a new version of the patch. I've tested on Linux and AIX.
> There are still some tests failing but it starts having a good shape ! 
> However, I have few questions:
>
> 1) locale.name and syscalls

just a terminology nit: none of those are syscalls.

> 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.

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.

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

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.

> 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.

> 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.

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.

With the attached patch, the code compiled using
--enable-clocale=ieee_1003.1-2008.

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

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: lx7.patch --]
[-- Type: text/x-patch, Size: 1783 bytes --]

diff --git a/libstdc++-v3/config/locale/ieee_1003.1-2008/ctype_members.cc b/libstdc++-v3/config/locale/ieee_1003.1-2008/ctype_members.cc
--- a/libstdc++-v3/config/locale/ieee_1003.1-2008/ctype_members.cc
+++ b/libstdc++-v3/config/locale/ieee_1003.1-2008/ctype_members.cc
@@ -196,7 +196,7 @@ namespace std _GLIBCXX_VISIBILITY(defaul
   do_is(mask __m, char_type __c) const
   {
     bool __ret = false;
-    const size_t __bitmasksize = 11;
+    const size_t __bitmasksize = 15;
 #ifndef _GLIBCXX_HAVE_ISWCTYPE_L
     __c_locale __old = uselocale((locale_t)_M_c_locale_ctype);
 #endif
@@ -227,7 +227,7 @@ namespace std _GLIBCXX_VISIBILITY(defaul
 #endif
     for (;__lo < __hi; ++__vec, ++__lo)
       {
-	const size_t __bitmasksize = 11;
+	const size_t __bitmasksize = 15;
 	mask __m = 0;
 	for (size_t __bitcur = 0; __bitcur <= __bitmasksize; ++__bitcur)
 #ifdef _GLIBCXX_HAVE_ISWCTYPE_L
@@ -345,7 +345,7 @@ namespace std _GLIBCXX_VISIBILITY(defaul
 	 __j < sizeof(_M_widen) / sizeof(wint_t); ++__j)
       _M_widen[__j] = btowc(__j);
 
-    for (size_t __k = 0; __k <= 11; ++__k)
+    for (size_t __k = 0; __k <= 15; ++__k)
       {
 	_M_bit[__k] = static_cast<mask>(_ISbit(__k));
 	_M_wmask[__k] = _M_convert_to_wmask(_M_bit[__k]);
diff --git a/libstdc++-v3/config/locale/ieee_1003.1-2008/monetary_members.cc b/libstdc++-v3/config/locale/ieee_1003.1-2008/monetary_members.cc
--- a/libstdc++-v3/config/locale/ieee_1003.1-2008/monetary_members.cc
+++ b/libstdc++-v3/config/locale/ieee_1003.1-2008/monetary_members.cc
@@ -29,6 +29,8 @@
 // Written by Benjamin Kosnik <bkoz@redhat.com>
 // Modified for DragonFly by John Marino <gnugcc@marino.st>
 
+// Solaris 11.4 doesn't make C99 members of struct lconv visible for C++11
+// without this.
 #include <locale>
 #include <cstring>