[PATCH] strfmon_l: Use specified locale for number formatting [BZ #19633]

[PATCH] strfmon_l: Use specified locale for number formatting [BZ #19633]

[Florian Weimer]

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

strfmon_l incorrectly applied global locale settings to number
formatting because printf_fp could only look at the global locale.

The new test tries to exercise the (in)dependence of a few LC_MONETARY
properties.  Some of the locales I used for testing might be buggy.

I introduced new helper macros to index the locale data in a locale_t
object.  A future cleanup opportunity is the _NL_CURRENT redefinition in
strfmon_l itself.  __guess_grouping should be moved out of printf_fp.c,
with a proper prototype in a header file.

I saw some ldbl_hidden_def thing.  I fear it is used by ld-as-a-functor
magic to work around the lack of templates in C.  What I did should
hopefully be safe.

Martin, does this patch match what you had in mind?

Thanks,
Florian

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-strfmon_l-Use-specified-locale-for-number-formatting.patch --]
[-- Type: text/x-patch; name="0001-strfmon_l-Use-specified-locale-for-number-formatting.patch", Size: 15635 bytes --]

2016-02-25  Florian Weimer  <fweimer@redhat.com>

	[BZ #19633]
	Use specified locale for number formatting in strfmon_l.
	* locale/localeinfo.h (__nl_lookup, _nl_lookup_wstr)
	(__nl_lookup_word): New inline functions.
	* include/printf.h (__print_fp_l): Declare.
	* stdio-common/printf_fp.c (___printf_fp_l): Renamed from
	___printf_fp.  Add locale argument.  Replace _NL_CURRENT with
	_nl_lookup and _NL_CURRENT_WORD with _nl_lookup_word.
	(___printf_fp): New function.
	* stdlib/strfmon_l.c (__printf_fp): Remove declaration.
	(__vstrfmon_l): Call __printf_fp_l instead of printf_fp.
	* stdlib/tst-strfmon_l.c (do_test): New test.
	* stdlib/Makefile (tests): Add kt.
	(LOCALES): Build additional locales.
	(tst-strfmon_l.out): Require locales.

diff --git a/include/printf.h b/include/printf.h
index c0bd2d2..b12b5dc 100644
--- a/include/printf.h
+++ b/include/printf.h
@@ -1,6 +1,7 @@
 #ifndef	_PRINTF_H
 
 #include <stdio-common/printf.h>
+#include <xlocale.h>
 
 /* Now define the internal interfaces.  */
 extern int __printf_fphex (FILE *, const struct printf_info *,
@@ -8,5 +9,8 @@ extern int __printf_fphex (FILE *, const struct printf_info *,
 extern int __printf_fp (FILE *, const struct printf_info *,
 			const void *const *);
 libc_hidden_proto (__printf_fp)
+extern int __printf_fp_l (FILE *, locale_t, const struct printf_info *,
+			  const void *const *);
+libc_hidden_proto (__printf_fp_l)
 
 #endif
diff --git a/locale/localeinfo.h b/locale/localeinfo.h
index 5c4e6ef..94627f3 100644
--- a/locale/localeinfo.h
+++ b/locale/localeinfo.h
@@ -299,6 +299,27 @@ extern __thread struct __locale_data *const *_nl_current_##category \
 
 #endif
 
+/* Extract CATEGORY locale's string for ITEM.  */
+static inline const char *
+_nl_lookup (locale_t l, int category, int item)
+{
+  return l->__locales[category]->values[_NL_ITEM_INDEX (item)].string;
+}
+
+/* Extract CATEGORY locale's wide string for ITEM.  */
+static inline const wchar_t *
+_nl_lookup_wstr (locale_t l, int category, int item)
+{
+  return (wchar_t *) l->__locales[category]
+    ->values[_NL_ITEM_INDEX (item)].wstr;
+}
+
+/* Extract the CATEGORY locale's word for ITEM.  */
+static inline uint32_t
+_nl_lookup_word (locale_t l, int category, int item)
+{
+  return l->__locales[category]->values[_NL_ITEM_INDEX (item)].word;
+}
 
 /* Default search path if no LOCPATH environment variable.  */
 extern const char _nl_default_locale_path[] attribute_hidden;
diff --git a/stdio-common/printf_fp.c b/stdio-common/printf_fp.c
index 4134f8a..baada9e 100644
--- a/stdio-common/printf_fp.c
+++ b/stdio-common/printf_fp.c
@@ -209,9 +209,9 @@ hack_digit (struct hack_digit_param *p)
 }
 
 int
-___printf_fp (FILE *fp,
-	      const struct printf_info *info,
-	      const void *const *args)
+___printf_fp_l (FILE *fp, locale_t loc,
+		const struct printf_info *info,
+		const void *const *args)
 {
   /* The floating-point value to output.  */
   union
@@ -263,18 +263,19 @@ ___printf_fp (FILE *fp,
   /* Figure out the decimal point character.  */
   if (info->extra == 0)
     {
-      decimal = _NL_CURRENT (LC_NUMERIC, DECIMAL_POINT);
-      decimalwc = _NL_CURRENT_WORD (LC_NUMERIC, _NL_NUMERIC_DECIMAL_POINT_WC);
+      decimal = _nl_lookup (loc, LC_NUMERIC, DECIMAL_POINT);
+      decimalwc = _nl_lookup_word
+	(loc, LC_NUMERIC, _NL_NUMERIC_DECIMAL_POINT_WC);
     }
   else
     {
-      decimal = _NL_CURRENT (LC_MONETARY, MON_DECIMAL_POINT);
+      decimal = _nl_lookup (loc, LC_MONETARY, MON_DECIMAL_POINT);
       if (*decimal == '\0')
-	decimal = _NL_CURRENT (LC_NUMERIC, DECIMAL_POINT);
-      decimalwc = _NL_CURRENT_WORD (LC_MONETARY,
+	decimal = _nl_lookup (loc, LC_NUMERIC, DECIMAL_POINT);
+      decimalwc = _nl_lookup_word (loc, LC_MONETARY,
 				    _NL_MONETARY_DECIMAL_POINT_WC);
       if (decimalwc == L'\0')
-	decimalwc = _NL_CURRENT_WORD (LC_NUMERIC,
+	decimalwc = _nl_lookup_word (loc, LC_NUMERIC,
 				      _NL_NUMERIC_DECIMAL_POINT_WC);
     }
   /* The decimal point character must not be zero.  */
@@ -284,9 +285,9 @@ ___printf_fp (FILE *fp,
   if (info->group)
     {
       if (info->extra == 0)
-	grouping = _NL_CURRENT (LC_NUMERIC, GROUPING);
+	grouping = _nl_lookup (loc, LC_NUMERIC, GROUPING);
       else
-	grouping = _NL_CURRENT (LC_MONETARY, MON_GROUPING);
+	grouping = _nl_lookup (loc, LC_MONETARY, MON_GROUPING);
 
       if (*grouping <= 0 || *grouping == CHAR_MAX)
 	grouping = NULL;
@@ -296,19 +297,20 @@ ___printf_fp (FILE *fp,
 	  if (wide)
 	    {
 	      if (info->extra == 0)
-		thousands_sepwc =
-		  _NL_CURRENT_WORD (LC_NUMERIC, _NL_NUMERIC_THOUSANDS_SEP_WC);
+		thousands_sepwc = _nl_lookup_word
+		  (loc, LC_NUMERIC, _NL_NUMERIC_THOUSANDS_SEP_WC);
 	      else
 		thousands_sepwc =
-		  _NL_CURRENT_WORD (LC_MONETARY,
+		  _nl_lookup_word (loc, LC_MONETARY,
 				    _NL_MONETARY_THOUSANDS_SEP_WC);
 	    }
 	  else
 	    {
 	      if (info->extra == 0)
-		thousands_sep = _NL_CURRENT (LC_NUMERIC, THOUSANDS_SEP);
+		thousands_sep = _nl_lookup (loc, LC_NUMERIC, THOUSANDS_SEP);
 	      else
-		thousands_sep = _NL_CURRENT (LC_MONETARY, MON_THOUSANDS_SEP);
+		thousands_sep = _nl_lookup
+		  (loc, LC_MONETARY, MON_THOUSANDS_SEP);
 	    }
 
 	  if ((wide && thousands_sepwc == L'\0')
@@ -1171,9 +1173,11 @@ ___printf_fp (FILE *fp,
 	  size_t decimal_len;
 	  size_t thousands_sep_len;
 	  wchar_t *copywc;
-	  size_t factor = (info->i18n
-			   ? _NL_CURRENT_WORD (LC_CTYPE, _NL_CTYPE_MB_CUR_MAX)
-			   : 1);
+	  size_t factor;
+	  if (info->i18n)
+	    factor = _nl_lookup_word (loc, LC_CTYPE, _NL_CTYPE_MB_CUR_MAX);
+	  else
+	    factor = 1;
 
 	  decimal_len = strlen (decimal);
 
@@ -1244,8 +1248,18 @@ ___printf_fp (FILE *fp,
   }
   return done;
 }
+ldbl_hidden_def (___printf_fp_l, __printf_fp_l)
+ldbl_strong_alias (___printf_fp_l, __printf_fp_l)
+
+int
+___printf_fp (FILE *fp, const struct printf_info *info,
+	      const void *const *args)
+{
+  return ___printf_fp_l (fp, _NL_CURRENT_LOCALE, info, args);
+}
 ldbl_hidden_def (___printf_fp, __printf_fp)
 ldbl_strong_alias (___printf_fp, __printf_fp)
+
 \f
 /* Return the number of extra grouping characters that will be inserted
    into a number with INTDIG_MAX integer digits.  */
diff --git a/stdlib/Makefile b/stdlib/Makefile
index 26fe67a..d978774 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -76,7 +76,7 @@ tests		:= tst-strtol tst-strtod testmb testrand testsort testdiv   \
 		   tst-secure-getenv tst-strtod-overflow tst-strtod-round   \
 		   tst-tininess tst-strtod-underflow tst-tls-atexit	    \
 		   tst-setcontext3 tst-tls-atexit-nodelete		    \
-		   tst-strtol-locale tst-strtod-nan-locale
+		   tst-strtol-locale tst-strtod-nan-locale tst-strfmon_l
 tests-static	:= tst-secure-getenv
 
 modules-names	= tst-tls-atexit-lib
@@ -126,7 +126,8 @@ include ../Rules
 
 ifeq ($(run-built-tests),yes)
 LOCALES := cs_CZ.UTF-8 de_DE.UTF-8 en_US.ISO-8859-1 tr_TR.UTF-8 \
-	   tr_TR.ISO-8859-9
+	   tr_TR.ISO-8859-9 tg_TJ.UTF-8 te_IN.UTF-8 bn_IN.UTF-8 \
+	   el_GR.UTF-8
 include ../gen-locales.mk
 
 $(objpfx)bug-strtod2.out: $(gen-locales)
@@ -137,6 +138,7 @@ $(objpfx)tst-strtod4.out: $(gen-locales)
 $(objpfx)tst-strtod5.out: $(gen-locales)
 $(objpfx)tst-strtol-locale.out: $(gen-locales)
 $(objpfx)tst-strtod-nan-locale.out: $(gen-locales)
+$(objpfx)tst-strfmon_l.out: $(gen-locales)
 endif
 
 # Testdir has to be named stdlib and needs to be writable
diff --git a/stdlib/strfmon_l.c b/stdlib/strfmon_l.c
index b357020..5851a5b 100644
--- a/stdlib/strfmon_l.c
+++ b/stdlib/strfmon_l.c
@@ -68,9 +68,6 @@
 #define _NL_CURRENT(category, item) \
   (current->values[_NL_ITEM_INDEX (item)].string)
 
-extern int __printf_fp (FILE *, const struct printf_info *,
-			const void *const *);
-libc_hidden_proto (__printf_fp)
 /* This function determines the number of digit groups in the output.
    The definition is in printf_fp.c.  */
 extern unsigned int __guess_grouping (unsigned int intdig_max,
@@ -532,7 +529,7 @@ __vstrfmon_l (char *s, size_t maxsize, __locale_t loc, const char *format,
       info.extra = 1;		/* This means use values from LC_MONETARY.  */
 
       ptr = &fpnum;
-      done = __printf_fp (&f._sbf._f, &info, &ptr);
+      done = __printf_fp_l (&f._sbf._f, loc, &info, &ptr);
       if (done < 0)
 	return -1;
 
diff --git a/stdlib/tst-strfmon_l.c b/stdlib/tst-strfmon_l.c
new file mode 100644
index 0000000..867e576
--- /dev/null
+++ b/stdlib/tst-strfmon_l.c
@@ -0,0 +1,185 @@
+/* Test locale dependence of strfmon_l.
+   Copyright (C) 2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <stdbool.h>
+#include <stdio.h>
+#include <monetary.h>
+#include <string.h>
+#include <stdlib.h>
+#include <locale.h>
+
+static const char *const en_us_name = "en_US.ISO-8859-1";
+
+static locale_t loc;
+
+static void
+init_loc (const char *global_name, const char *local_name)
+{
+  loc = newlocale (LC_ALL_MASK, local_name, 0);
+  if (loc == 0)
+    {
+      printf ("error: newlocale (%s): %m\n", local_name);
+      abort ();
+    }
+
+  if (setlocale (LC_ALL, global_name) == NULL)
+    {
+      printf ("error: setlocale (%s): %m\n", global_name);
+      abort ();
+    }
+}
+
+static bool errors;
+static char actual[64];
+
+struct testcase
+{
+  int result;
+  const char *format;
+  const char *expected;
+};
+
+static void
+check (int result, const char *format, const char *expected)
+{
+  if (result < 0)
+    {
+      printf ("error: format \"%s\": strfmon_l: %m\n", format);
+      errors = true;
+    }
+  else if (strcmp (actual, expected) != 0)
+    {
+      printf ("error: format \"%s\": mismatch\n", format);
+      printf ("error:   expected: \"%s\"\n", expected);
+      printf ("error:   actual:   \"%s\"\n", actual);
+      errors = true;
+    }
+}
+
+static void
+test_en_us (const char *other_name)
+{
+  init_loc (other_name, en_us_name);
+  check (strfmon_l (actual, sizeof (actual), loc, "%i", 1234567.89),
+         "%i", "USD 1,234,567.89");
+  check (strfmon_l (actual, sizeof (actual), loc, "%n", 1234567.89),
+         "%n", "$1,234,567.89");
+  check (strfmon_l (actual, sizeof (actual), loc, "%^i", 1234567.89),
+         "%^i", "USD 1234567.89");
+  check (strfmon_l (actual, sizeof (actual), loc, "%^n", 1234567.89),
+         "%^n", "$1234567.89");
+  check (strfmon_l (actual, sizeof (actual), loc, "%i", -1234567.89),
+         "%i", "-USD 1,234,567.89");
+  check (strfmon_l (actual, sizeof (actual), loc, "%n", -1234567.89),
+         "%n", "-$1,234,567.89");
+  check (strfmon_l (actual, sizeof (actual), loc, "%^i", -1234567.89),
+         "%^i", "-USD 1234567.89");
+  check (strfmon_l (actual, sizeof (actual), loc, "%^n", -1234567.89),
+         "%^n", "-$1234567.89");
+  freelocale (loc);
+}
+
+static int
+do_test (void)
+{
+  const char *other_name = "de_DE.UTF-8";
+  init_loc (en_us_name, other_name);
+  check (strfmon_l (actual, sizeof (actual), loc, "%i", 1234567.89),
+         "%i", "1.234.567,89 EUR");
+  check (strfmon_l (actual, sizeof (actual), loc, "%n", 1234567.89),
+         "%n", "1.234.567,89 \u20ac");
+  check (strfmon_l (actual, sizeof (actual), loc, "%^i", 1234567.89),
+         "%^i", "1234567,89 EUR");
+  check (strfmon_l (actual, sizeof (actual), loc, "%^n", 1234567.89),
+         "%^n", "1234567,89 \u20ac");
+  check (strfmon_l (actual, sizeof (actual), loc, "%i", -1234567.89),
+         "%i", "-1.234.567,89 EUR");
+  check (strfmon_l (actual, sizeof (actual), loc, "%n", -1234567.89),
+         "%n", "-1.234.567,89 \u20ac");
+  check (strfmon_l (actual, sizeof (actual), loc, "%^i", -1234567.89),
+         "%^i", "-1234567,89 EUR");
+  check (strfmon_l (actual, sizeof (actual), loc, "%^n", -1234567.89),
+         "%^n", "-1234567,89 \u20ac");
+  freelocale (loc);
+  test_en_us (other_name);
+
+  other_name = "tg_TJ.UTF-8";
+  init_loc (en_us_name, other_name);
+  check (strfmon_l (actual, sizeof (actual), loc, "%i", 1234567.89),
+         "%i", "1 234 567.89 TJS");
+  check (strfmon_l (actual, sizeof (actual), loc, "%n", 1234567.89),
+         "%n", "1 234 567.89 \u0440\u0443\u0431");
+  check (strfmon_l (actual, sizeof (actual), loc, "%^i", 1234567.89),
+         "%^i", "1234567.89 TJS");
+  check (strfmon_l (actual, sizeof (actual), loc, "%^n", 1234567.89),
+         "%^n", "1234567.89 \u0440\u0443\u0431");
+  freelocale (loc);
+  test_en_us (other_name);
+
+  other_name = "te_IN.UTF-8";
+  init_loc (en_us_name, other_name);
+  check (strfmon_l (actual, sizeof (actual), loc, "%i", 1234567.89),
+         "%i", "INR12,34,567.89");
+  check (strfmon_l (actual, sizeof (actual), loc, "%n", 1234567.89),
+         "%n", "\u20b912,34,567.89");
+  check (strfmon_l (actual, sizeof (actual), loc, "%^i", 1234567.89),
+         "%^i", "INR1234567.89");
+  check (strfmon_l (actual, sizeof (actual), loc, "%^n", 1234567.89),
+         "%^n", "\u20b91234567.89");
+  freelocale (loc);
+  test_en_us (other_name);
+
+  other_name = "bn_IN.UTF-8";
+  init_loc (en_us_name, other_name);
+  check (strfmon_l (actual, sizeof (actual), loc, "%i", 1234567.89),
+         "%i", "INR 12,345,67.89");
+  check (strfmon_l (actual, sizeof (actual), loc, "%n", 1234567.89),
+         "%n", "\u20b9 12,345,67.89");
+  check (strfmon_l (actual, sizeof (actual), loc, "%^i", 1234567.89),
+         "%^i", "INR 1234567.89");
+  check (strfmon_l (actual, sizeof (actual), loc, "%^n", 1234567.89),
+         "%^n", "\u20b9 1234567.89");
+  freelocale (loc);
+  test_en_us (other_name);
+
+  other_name = "el_GR.UTF-8";
+  init_loc (en_us_name, other_name);
+  check (strfmon_l (actual, sizeof (actual), loc, "%i", 1234567.89),
+         "%i", "1.234.567,89EUR");
+  check (strfmon_l (actual, sizeof (actual), loc, "%n", 1234567.89),
+         "%n", "1.234.567,89\u20ac");
+  check (strfmon_l (actual, sizeof (actual), loc, "%^i", 1234567.89),
+         "%^i", "1234567,89EUR");
+  check (strfmon_l (actual, sizeof (actual), loc, "%^n", 1234567.89),
+         "%^n", "1234567,89\u20ac");
+  check (strfmon_l (actual, sizeof (actual), loc, "%i", -1234567.89),
+         "%i", "-EUR1.234.567,89");
+  check (strfmon_l (actual, sizeof (actual), loc, "%n", -1234567.89),
+         "%n", "-\u20ac1.234.567,89");
+  check (strfmon_l (actual, sizeof (actual), loc, "%^i", -1234567.89),
+         "%^i", "-EUR1234567,89");
+  check (strfmon_l (actual, sizeof (actual), loc, "%^n", -1234567.89),
+         "%^n", "-\u20ac1234567,89");
+  freelocale (loc);
+  test_en_us (other_name);
+
+  return errors;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"

Re: [PATCH] strfmon_l: Use specified locale for number formatting [BZ #19633]

[Martin Sebor]

On 02/25/2016 01:23 PM, Florian Weimer wrote:
> strfmon_l incorrectly applied global locale settings to number
> formatting because printf_fp could only look at the global locale.
>
> The new test tries to exercise the (in)dependence of a few LC_MONETARY
> properties.  Some of the locales I used for testing might be buggy.
>
> I introduced new helper macros to index the locale data in a locale_t
> object.  A future cleanup opportunity is the _NL_CURRENT redefinition in
> strfmon_l itself.  __guess_grouping should be moved out of printf_fp.c,
> with a proper prototype in a header file.
>
> I saw some ldbl_hidden_def thing.  I fear it is used by ld-as-a-functor
> magic to work around the lack of templates in C.  What I did should
> hopefully be safe.
>
> Martin, does this patch match what you had in mind?

It's very close to what I started with.  I had made some slightly
different choices in my initial approach, some of which would have
probably turned out to be incorrect in the end.  I like that you
added inline functions rather than macros to access the locale
data.  That's a lot cleaner.

The one thing that I'm curious about is the two sets of
_NL_CURRENT_XXX() macros defined in localeinfo.h and controlled
by NL_CURRENT_INDIRECT.  If I'm reading your patch right it
assumes NL_CURRENT_INDIRECT is undefined.  Does it still work
as expected when NL_CURRENT_INDIRECT is defined?  (I never got
far enough along to test this with my patch.)

Other than that, I have one suggestion for the test.  I don't
know if it would be in line with the glibc approach to testing
(and it's also a matter of personal preference), but for what
it's worth, I find data/table driven tests easier to read, work
with, and enhance.  I.e., rather than calling

   check (strfmon_l (actual, sizeof (actual), loc, "%i", 1234567.89),
          "%i", "USD 1,234,567.89");

a bunch of times with varying formats and data, I find that
defining an array of structs containing the data and then
iterating over the data makes it easier to see the important
bits under test and keep the unimportant parts out of sight:

   struct {
       const char *format;
       double value;
       const char *expect;
   } testdata[] = {
       "%i", 1234567.89, "USD 1,234,567.89",
       ...
   };

   for (i = 0; i != sizeof testdata / sizeof *testdata; ++i)
       check (strfmon_l (actual, sizeof (actual), loc,
                         tesdata[i].format, testdata[i].value),
                         tesdata[i].format, testdata[i].expect);

Martin

Re: [PATCH] strfmon_l: Use specified locale for number formatting [BZ #19633]

[Florian Weimer]

On 02/25/2016 11:16 PM, Martin Sebor wrote:
> On 02/25/2016 01:23 PM, Florian Weimer wrote:
>> strfmon_l incorrectly applied global locale settings to number
>> formatting because printf_fp could only look at the global locale.
>>
>> The new test tries to exercise the (in)dependence of a few LC_MONETARY
>> properties.  Some of the locales I used for testing might be buggy.
>>
>> I introduced new helper macros to index the locale data in a locale_t
>> object.  A future cleanup opportunity is the _NL_CURRENT redefinition in
>> strfmon_l itself.  __guess_grouping should be moved out of printf_fp.c,
>> with a proper prototype in a header file.
>>
>> I saw some ldbl_hidden_def thing.  I fear it is used by ld-as-a-functor
>> magic to work around the lack of templates in C.  What I did should
>> hopefully be safe.
>>
>> Martin, does this patch match what you had in mind?
> 
> It's very close to what I started with.  I had made some slightly
> different choices in my initial approach, some of which would have
> probably turned out to be incorrect in the end.  I like that you
> added inline functions rather than macros to access the locale
> data.  That's a lot cleaner.

Thanks for your comments.

Yes, it makes the types nicely explicit.

> The one thing that I'm curious about is the two sets of
> _NL_CURRENT_XXX() macros defined in localeinfo.h and controlled
> by NL_CURRENT_INDIRECT.  If I'm reading your patch right it
> assumes NL_CURRENT_INDIRECT is undefined.  Does it still work
> as expected when NL_CURRENT_INDIRECT is defined?  (I never got
> far enough along to test this with my patch.)

I don't think it does.  The representation of locale_t does not really
change due to static linking.

I have added a static test, like this:

/* Test locale dependence of strfmon_l, static variant.
   Copyright (C) 2016 Free Software Foundation, Inc.
   This file is part of the GNU C Library.
…
   You should have received a copy of the GNU Lesser General Public
   License along with the GNU C Library; if not, see
   <http://www.gnu.org/licenses/>.  */

#include "tst-strfmon_l.c"

stdlib/tst-strfmon_l-static: ELF 64-bit LSB executable, x86-64, version
1 (GNU/Linux), statically linked, for GNU/Linux 2.6.32,
BuildID[sha1]=a78456f193cfcd0b04912f55654c7cd342edca24, not stripped

And it still passes.  I can include it in the change if you think it's
necessary.

> Other than that, I have one suggestion for the test.  I don't
> know if it would be in line with the glibc approach to testing
> (and it's also a matter of personal preference), but for what
> it's worth, I find data/table driven tests easier to read, work
> with, and enhance.  I.e., rather than calling
> 
>   check (strfmon_l (actual, sizeof (actual), loc, "%i", 1234567.89),
>          "%i", "USD 1,234,567.89");
> 
> a bunch of times with varying formats and data, I find that
> defining an array of structs containing the data and then
> iterating over the data makes it easier to see the important
> bits under test and keep the unimportant parts out of sight:

I did not do this to avoid a warning when building with -Wformat-nonliteral.

Florian

Re: [PATCH] strfmon_l: Use specified locale for number formatting [BZ #19633]

[Carlos O'Donell]

On 03/07/2016 08:22 AM, Florian Weimer wrote:
> On 02/25/2016 11:16 PM, Martin Sebor wrote:
>> On 02/25/2016 01:23 PM, Florian Weimer wrote:
>>> strfmon_l incorrectly applied global locale settings to number
>>> formatting because printf_fp could only look at the global locale.
>>>
>>> The new test tries to exercise the (in)dependence of a few LC_MONETARY
>>> properties.  Some of the locales I used for testing might be buggy.
>>>
>>> I introduced new helper macros to index the locale data in a locale_t
>>> object.  A future cleanup opportunity is the _NL_CURRENT redefinition in
>>> strfmon_l itself.  __guess_grouping should be moved out of printf_fp.c,
>>> with a proper prototype in a header file.
>>>
>>> I saw some ldbl_hidden_def thing.  I fear it is used by ld-as-a-functor
>>> magic to work around the lack of templates in C.  What I did should
>>> hopefully be safe.
>>>
>>> Martin, does this patch match what you had in mind?
>>
>> It's very close to what I started with.  I had made some slightly
>> different choices in my initial approach, some of which would have
>> probably turned out to be incorrect in the end.  I like that you
>> added inline functions rather than macros to access the locale
>> data.  That's a lot cleaner.
> 
> Thanks for your comments.
> 
> Yes, it makes the types nicely explicit.
> 
>> The one thing that I'm curious about is the two sets of
>> _NL_CURRENT_XXX() macros defined in localeinfo.h and controlled
>> by NL_CURRENT_INDIRECT.  If I'm reading your patch right it
>> assumes NL_CURRENT_INDIRECT is undefined.  Does it still work
>> as expected when NL_CURRENT_INDIRECT is defined?  (I never got
>> far enough along to test this with my patch.)
> 
> I don't think it does.  The representation of locale_t does not really
> change due to static linking.
> 
> I have added a static test, like this:
> 
> /* Test locale dependence of strfmon_l, static variant.
>    Copyright (C) 2016 Free Software Foundation, Inc.
>    This file is part of the GNU C Library.
> …
>    You should have received a copy of the GNU Lesser General Public
>    License along with the GNU C Library; if not, see
>    <http://www.gnu.org/licenses/>.  */
> 
> #include "tst-strfmon_l.c"
> 
> stdlib/tst-strfmon_l-static: ELF 64-bit LSB executable, x86-64, version
> 1 (GNU/Linux), statically linked, for GNU/Linux 2.6.32,
> BuildID[sha1]=a78456f193cfcd0b04912f55654c7cd342edca24, not stripped
> 
> And it still passes.  I can include it in the change if you think it's
> necessary.
> 
>> Other than that, I have one suggestion for the test.  I don't
>> know if it would be in line with the glibc approach to testing
>> (and it's also a matter of personal preference), but for what
>> it's worth, I find data/table driven tests easier to read, work
>> with, and enhance.  I.e., rather than calling
>>
>>   check (strfmon_l (actual, sizeof (actual), loc, "%i", 1234567.89),
>>          "%i", "USD 1,234,567.89");
>>
>> a bunch of times with varying formats and data, I find that
>> defining an array of structs containing the data and then
>> iterating over the data makes it easier to see the important
>> bits under test and keep the unimportant parts out of sight:
> 
> I did not do this to avoid a warning when building with -Wformat-nonliteral.

I tend to agree with Martin here, having that separation between
test and data makes it easier to read and change the test or add
more tests.

If you're getting a warning from the compiler you expect but don't
care about then you can just silence the warning with the appropriate
attribute?

-- 
Cheers,
Carlos.

Re: [PATCH] strfmon_l: Use specified locale for number formatting [BZ #19633]

[Florian Weimer]

On 03/07/2016 04:28 PM, Carlos O'Donell wrote:

> I tend to agree with Martin here, having that separation between
> test and data makes it easier to read and change the test or add
> more tests.
> 
> If you're getting a warning from the compiler you expect but don't
> care about then you can just silence the warning with the appropriate
> attribute?

The warning is not enabled by default (or even -W), so I'm not sure if
that's even necessary.

What's your opinion on the separate static test?  Should I include it?

Thanks,
Florian

Re: [PATCH] strfmon_l: Use specified locale for number formatting [BZ #19633]

[Carlos O'Donell]

On 03/07/2016 10:58 AM, Florian Weimer wrote:
> On 03/07/2016 04:28 PM, Carlos O'Donell wrote:
> 
>> I tend to agree with Martin here, having that separation between
>> test and data makes it easier to read and change the test or add
>> more tests.
>>
>> If you're getting a warning from the compiler you expect but don't
>> care about then you can just silence the warning with the appropriate
>> attribute?
> 
> The warning is not enabled by default (or even -W), so I'm not sure if
> that's even necessary.

If the warning isn't enabled, then we don't need to worry about it today.
The vision here is that we can run everything with -Werror, but we aren't
there yet in some cases (see -Wundef fixes required).

> What's your opinion on the separate static test?  Should I include it?

I have no strong opinion. The dynamic test is sufficient. I don't see
that much reward to a static test other than perhaps to expose problems
we have in the static support via NL_CURRENT_INDIRECT?

-- 
Cheers,
Carlos.

Re: [PATCH] strfmon_l: Use specified locale for number formatting [BZ #19633]

[Joseph Myers]

On Mon, 7 Mar 2016, Carlos O'Donell wrote:

> If the warning isn't enabled, then we don't need to worry about it today.
> The vision here is that we can run everything with -Werror, but we aren't
> there yet in some cases (see -Wundef fixes required).

We no longer use -Wno-error=undef as of:

commit 48bb14bdbbeb09cb3cd950d7346688958f1bce1a
Author: Joseph Myers <joseph@codesourcery.com>
Date:   Thu Aug 20 20:50:05 2015 +0000

    Don't use -Wno-error=undef.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] strfmon_l: Use specified locale for number formatting [BZ #19633]

[Florian Weimer]

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

On 03/07/2016 05:58 PM, Carlos O'Donell wrote:
> On 03/07/2016 10:58 AM, Florian Weimer wrote:
>> On 03/07/2016 04:28 PM, Carlos O'Donell wrote:
>>
>>> I tend to agree with Martin here, having that separation between
>>> test and data makes it easier to read and change the test or add
>>> more tests.
>>>
>>> If you're getting a warning from the compiler you expect but don't
>>> care about then you can just silence the warning with the appropriate
>>> attribute?
>>
>> The warning is not enabled by default (or even -W), so I'm not sure if
>> that's even necessary.
> 
> If the warning isn't enabled, then we don't need to worry about it today.
> The vision here is that we can run everything with -Werror, but we aren't
> there yet in some cases (see -Wundef fixes required).

Okay, here is a table-based version of the test.  Other parts of the
patch are unchanged.

Florian

[-- Attachment #2: 0001-strfmon_l-Use-specified-locale-for-number-formatting.patch --]
[-- Type: text/x-patch, Size: 14319 bytes --]

2016-02-25  Florian Weimer  <fweimer@redhat.com>

	[BZ #19633]
	Use specified locale for number formatting in strfmon_l.
	* locale/localeinfo.h (__nl_lookup, _nl_lookup_wstr)
	(__nl_lookup_word): New inline functions.
	* include/printf.h (__print_fp_l): Declare.
	* stdio-common/printf_fp.c (___printf_fp_l): Renamed from
	___printf_fp.  Add locale argument.  Replace _NL_CURRENT with
	_nl_lookup and _NL_CURRENT_WORD with _nl_lookup_word.
	(___printf_fp): New function.
	* stdlib/strfmon_l.c (__printf_fp): Remove declaration.
	(__vstrfmon_l): Call __printf_fp_l instead of printf_fp.
	* stdlib/tst-strfmon_l.c (do_test): New test.
	* stdlib/Makefile (tests): Add kt.
	(LOCALES): Build additional locales.
	(tst-strfmon_l.out): Require locales.

diff --git a/include/printf.h b/include/printf.h
index c0bd2d2..b12b5dc 100644
--- a/include/printf.h
+++ b/include/printf.h
@@ -1,6 +1,7 @@
 #ifndef	_PRINTF_H
 
 #include <stdio-common/printf.h>
+#include <xlocale.h>
 
 /* Now define the internal interfaces.  */
 extern int __printf_fphex (FILE *, const struct printf_info *,
@@ -8,5 +9,8 @@ extern int __printf_fphex (FILE *, const struct printf_info *,
 extern int __printf_fp (FILE *, const struct printf_info *,
 			const void *const *);
 libc_hidden_proto (__printf_fp)
+extern int __printf_fp_l (FILE *, locale_t, const struct printf_info *,
+			  const void *const *);
+libc_hidden_proto (__printf_fp_l)
 
 #endif
diff --git a/locale/localeinfo.h b/locale/localeinfo.h
index 5c4e6ef..94627f3 100644
--- a/locale/localeinfo.h
+++ b/locale/localeinfo.h
@@ -299,6 +299,27 @@ extern __thread struct __locale_data *const *_nl_current_##category \
 
 #endif
 
+/* Extract CATEGORY locale's string for ITEM.  */
+static inline const char *
+_nl_lookup (locale_t l, int category, int item)
+{
+  return l->__locales[category]->values[_NL_ITEM_INDEX (item)].string;
+}
+
+/* Extract CATEGORY locale's wide string for ITEM.  */
+static inline const wchar_t *
+_nl_lookup_wstr (locale_t l, int category, int item)
+{
+  return (wchar_t *) l->__locales[category]
+    ->values[_NL_ITEM_INDEX (item)].wstr;
+}
+
+/* Extract the CATEGORY locale's word for ITEM.  */
+static inline uint32_t
+_nl_lookup_word (locale_t l, int category, int item)
+{
+  return l->__locales[category]->values[_NL_ITEM_INDEX (item)].word;
+}
 
 /* Default search path if no LOCPATH environment variable.  */
 extern const char _nl_default_locale_path[] attribute_hidden;
diff --git a/stdio-common/printf_fp.c b/stdio-common/printf_fp.c
index 4134f8a..baada9e 100644
--- a/stdio-common/printf_fp.c
+++ b/stdio-common/printf_fp.c
@@ -209,9 +209,9 @@ hack_digit (struct hack_digit_param *p)
 }
 
 int
-___printf_fp (FILE *fp,
-	      const struct printf_info *info,
-	      const void *const *args)
+___printf_fp_l (FILE *fp, locale_t loc,
+		const struct printf_info *info,
+		const void *const *args)
 {
   /* The floating-point value to output.  */
   union
@@ -263,18 +263,19 @@ ___printf_fp (FILE *fp,
   /* Figure out the decimal point character.  */
   if (info->extra == 0)
     {
-      decimal = _NL_CURRENT (LC_NUMERIC, DECIMAL_POINT);
-      decimalwc = _NL_CURRENT_WORD (LC_NUMERIC, _NL_NUMERIC_DECIMAL_POINT_WC);
+      decimal = _nl_lookup (loc, LC_NUMERIC, DECIMAL_POINT);
+      decimalwc = _nl_lookup_word
+	(loc, LC_NUMERIC, _NL_NUMERIC_DECIMAL_POINT_WC);
     }
   else
     {
-      decimal = _NL_CURRENT (LC_MONETARY, MON_DECIMAL_POINT);
+      decimal = _nl_lookup (loc, LC_MONETARY, MON_DECIMAL_POINT);
       if (*decimal == '\0')
-	decimal = _NL_CURRENT (LC_NUMERIC, DECIMAL_POINT);
-      decimalwc = _NL_CURRENT_WORD (LC_MONETARY,
+	decimal = _nl_lookup (loc, LC_NUMERIC, DECIMAL_POINT);
+      decimalwc = _nl_lookup_word (loc, LC_MONETARY,
 				    _NL_MONETARY_DECIMAL_POINT_WC);
       if (decimalwc == L'\0')
-	decimalwc = _NL_CURRENT_WORD (LC_NUMERIC,
+	decimalwc = _nl_lookup_word (loc, LC_NUMERIC,
 				      _NL_NUMERIC_DECIMAL_POINT_WC);
     }
   /* The decimal point character must not be zero.  */
@@ -284,9 +285,9 @@ ___printf_fp (FILE *fp,
   if (info->group)
     {
       if (info->extra == 0)
-	grouping = _NL_CURRENT (LC_NUMERIC, GROUPING);
+	grouping = _nl_lookup (loc, LC_NUMERIC, GROUPING);
       else
-	grouping = _NL_CURRENT (LC_MONETARY, MON_GROUPING);
+	grouping = _nl_lookup (loc, LC_MONETARY, MON_GROUPING);
 
       if (*grouping <= 0 || *grouping == CHAR_MAX)
 	grouping = NULL;
@@ -296,19 +297,20 @@ ___printf_fp (FILE *fp,
 	  if (wide)
 	    {
 	      if (info->extra == 0)
-		thousands_sepwc =
-		  _NL_CURRENT_WORD (LC_NUMERIC, _NL_NUMERIC_THOUSANDS_SEP_WC);
+		thousands_sepwc = _nl_lookup_word
+		  (loc, LC_NUMERIC, _NL_NUMERIC_THOUSANDS_SEP_WC);
 	      else
 		thousands_sepwc =
-		  _NL_CURRENT_WORD (LC_MONETARY,
+		  _nl_lookup_word (loc, LC_MONETARY,
 				    _NL_MONETARY_THOUSANDS_SEP_WC);
 	    }
 	  else
 	    {
 	      if (info->extra == 0)
-		thousands_sep = _NL_CURRENT (LC_NUMERIC, THOUSANDS_SEP);
+		thousands_sep = _nl_lookup (loc, LC_NUMERIC, THOUSANDS_SEP);
 	      else
-		thousands_sep = _NL_CURRENT (LC_MONETARY, MON_THOUSANDS_SEP);
+		thousands_sep = _nl_lookup
+		  (loc, LC_MONETARY, MON_THOUSANDS_SEP);
 	    }
 
 	  if ((wide && thousands_sepwc == L'\0')
@@ -1171,9 +1173,11 @@ ___printf_fp (FILE *fp,
 	  size_t decimal_len;
 	  size_t thousands_sep_len;
 	  wchar_t *copywc;
-	  size_t factor = (info->i18n
-			   ? _NL_CURRENT_WORD (LC_CTYPE, _NL_CTYPE_MB_CUR_MAX)
-			   : 1);
+	  size_t factor;
+	  if (info->i18n)
+	    factor = _nl_lookup_word (loc, LC_CTYPE, _NL_CTYPE_MB_CUR_MAX);
+	  else
+	    factor = 1;
 
 	  decimal_len = strlen (decimal);
 
@@ -1244,8 +1248,18 @@ ___printf_fp (FILE *fp,
   }
   return done;
 }
+ldbl_hidden_def (___printf_fp_l, __printf_fp_l)
+ldbl_strong_alias (___printf_fp_l, __printf_fp_l)
+
+int
+___printf_fp (FILE *fp, const struct printf_info *info,
+	      const void *const *args)
+{
+  return ___printf_fp_l (fp, _NL_CURRENT_LOCALE, info, args);
+}
 ldbl_hidden_def (___printf_fp, __printf_fp)
 ldbl_strong_alias (___printf_fp, __printf_fp)
+
 \f
 /* Return the number of extra grouping characters that will be inserted
    into a number with INTDIG_MAX integer digits.  */
diff --git a/stdlib/Makefile b/stdlib/Makefile
index 26fe67a..d978774 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -76,7 +76,7 @@ tests		:= tst-strtol tst-strtod testmb testrand testsort testdiv   \
 		   tst-secure-getenv tst-strtod-overflow tst-strtod-round   \
 		   tst-tininess tst-strtod-underflow tst-tls-atexit	    \
 		   tst-setcontext3 tst-tls-atexit-nodelete		    \
-		   tst-strtol-locale tst-strtod-nan-locale
+		   tst-strtol-locale tst-strtod-nan-locale tst-strfmon_l
 tests-static	:= tst-secure-getenv
 
 modules-names	= tst-tls-atexit-lib
@@ -126,7 +126,8 @@ include ../Rules
 
 ifeq ($(run-built-tests),yes)
 LOCALES := cs_CZ.UTF-8 de_DE.UTF-8 en_US.ISO-8859-1 tr_TR.UTF-8 \
-	   tr_TR.ISO-8859-9
+	   tr_TR.ISO-8859-9 tg_TJ.UTF-8 te_IN.UTF-8 bn_IN.UTF-8 \
+	   el_GR.UTF-8
 include ../gen-locales.mk
 
 $(objpfx)bug-strtod2.out: $(gen-locales)
@@ -137,6 +138,7 @@ $(objpfx)tst-strtod4.out: $(gen-locales)
 $(objpfx)tst-strtod5.out: $(gen-locales)
 $(objpfx)tst-strtol-locale.out: $(gen-locales)
 $(objpfx)tst-strtod-nan-locale.out: $(gen-locales)
+$(objpfx)tst-strfmon_l.out: $(gen-locales)
 endif
 
 # Testdir has to be named stdlib and needs to be writable
diff --git a/stdlib/strfmon_l.c b/stdlib/strfmon_l.c
index b357020..5851a5b 100644
--- a/stdlib/strfmon_l.c
+++ b/stdlib/strfmon_l.c
@@ -68,9 +68,6 @@
 #define _NL_CURRENT(category, item) \
   (current->values[_NL_ITEM_INDEX (item)].string)
 
-extern int __printf_fp (FILE *, const struct printf_info *,
-			const void *const *);
-libc_hidden_proto (__printf_fp)
 /* This function determines the number of digit groups in the output.
    The definition is in printf_fp.c.  */
 extern unsigned int __guess_grouping (unsigned int intdig_max,
@@ -532,7 +529,7 @@ __vstrfmon_l (char *s, size_t maxsize, __locale_t loc, const char *format,
       info.extra = 1;		/* This means use values from LC_MONETARY.  */
 
       ptr = &fpnum;
-      done = __printf_fp (&f._sbf._f, &info, &ptr);
+      done = __printf_fp_l (&f._sbf._f, loc, &info, &ptr);
       if (done < 0)
 	return -1;
 
diff --git a/stdlib/tst-strfmon_l.c b/stdlib/tst-strfmon_l.c
new file mode 100644
index 0000000..6841511
--- /dev/null
+++ b/stdlib/tst-strfmon_l.c
@@ -0,0 +1,220 @@
+/* Test locale dependence of strfmon_l.
+   Copyright (C) 2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include <stdbool.h>
+#include <stdio.h>
+#include <monetary.h>
+#include <string.h>
+#include <stdlib.h>
+#include <locale.h>
+
+static const char *const en_us_name = "en_US.ISO-8859-1";
+
+/* Locale value to be used by tests.  */
+static locale_t loc;
+static const char *loc_name;
+
+/* Set the global locale to GLOBAL_NAME, and the locale referenced by
+   the loc variable above to LOCAL_NAME.  */
+static void
+init_loc (const char *global_name, const char *local_name)
+{
+  loc = newlocale (LC_ALL_MASK, local_name, 0);
+  if (loc == 0)
+    {
+      printf ("error: newlocale (%s): %m\n", local_name);
+      abort ();
+    }
+  loc_name = local_name;
+
+  if (setlocale (LC_ALL, global_name) == NULL)
+    {
+      printf ("error: setlocale (%s): %m\n", global_name);
+      abort ();
+    }
+}
+
+/* Expected strings for a positive or negative value.  */
+struct testcase
+{
+  const char *i;                /* %i */
+  const char *n;                /* %n */
+  const char *i_ungrouped;      /* %^i */
+  const char *n_ungrouped;      /* %^n */
+};
+
+/* Collected expected strings for both positive and negative
+   values.  */
+struct testcase_pair
+{
+  struct testcase positive;     /* 1234567.89 */
+  struct testcase negative;     /* -1234567.89 */
+};
+
+static bool errors;
+
+/* Test one value using the locale loc.  */
+static void
+test_one (const char *format, double value, const char *expected)
+{
+  static char actual[64];
+  int result = strfmon_l (actual, sizeof (actual), loc, format, value);
+  if (result < 0)
+    {
+      printf ("error: locale %s, format \"%s\", value %g: strfmon_l: %m\n",
+              loc_name, format, value);
+      errors = true;
+    }
+  else if (strcmp (actual, expected) != 0)
+    {
+      printf ("error: locale %s, format \"%s\", value %g: mismatch\n",
+              loc_name, format, value);
+      printf ("error:   expected: \"%s\"\n", expected);
+      printf ("error:   actual:   \"%s\"\n", actual);
+      errors = true;
+    }
+}
+
+static void
+test_pair (const struct testcase_pair *pair)
+{
+  double positive = 1234567.89;
+  test_one ("%i", positive, pair->positive.i);
+  test_one ("%n", positive, pair->positive.n);
+  test_one ("%^i", positive, pair->positive.i_ungrouped);
+  test_one ("%^n", positive, pair->positive.n_ungrouped);
+  double negative = -1234567.89;
+  test_one ("%i", negative, pair->negative.i);
+  test_one ("%n", negative, pair->negative.n);
+  test_one ("%^i", negative, pair->negative.i_ungrouped);
+  test_one ("%^n", negative, pair->negative.n_ungrouped);
+}
+
+static const struct testcase_pair en_us =
+  {
+    {
+      "USD 1,234,567.89", "$1,234,567.89",
+      "USD 1234567.89", "$1234567.89"
+    },
+    {
+      "-USD 1,234,567.89", "-$1,234,567.89",
+      "-USD 1234567.89", "-$1234567.89"
+    }
+  };
+
+static void
+test_en_us (const char *other_name)
+{
+  init_loc (other_name, en_us_name);
+  test_pair (&en_us);
+  freelocale (loc);
+}
+
+struct locale_pair
+{
+  const char *locale_name;
+  struct testcase_pair pair;
+};
+
+static const struct locale_pair tests[] =
+  {
+    {
+      "de_DE.UTF-8",
+      {
+        {
+         "1.234.567,89 EUR", "1.234.567,89 \u20ac",
+         "1234567,89 EUR", "1234567,89 \u20ac"
+        },
+        {
+         "-1.234.567,89 EUR", "-1.234.567,89 \u20ac",
+         "-1234567,89 EUR", "-1234567,89 \u20ac"
+        }
+      },
+    },
+    {
+      "tg_TJ.UTF-8",
+      {
+        {
+          "1 234 567.89 TJS", "1 234 567.89 \u0440\u0443\u0431",
+          "1234567.89 TJS", "1234567.89 \u0440\u0443\u0431"
+        },
+        {
+          "-1 234 567.89 TJS", "-1 234 567.89 \u0440\u0443\u0431",
+          "-1234567.89 TJS", "-1234567.89 \u0440\u0443\u0431"
+        }
+      }
+    },
+    {
+      "te_IN.UTF-8",
+      {
+        {
+          "INR12,34,567.89", "\u20b912,34,567.89",
+          "INR1234567.89", "\u20b91234567.89"
+        },
+        {
+          "-INR12,34,567.89", "-\u20b912,34,567.89",
+          "-INR1234567.89", "-\u20b91234567.89"
+        }
+      }
+    },
+    {
+      "bn_IN.UTF-8",
+      {
+        {
+          "INR 12,345,67.89", "\u20b9 12,345,67.89",
+          "INR 1234567.89", "\u20b9 1234567.89"
+        },
+        {
+          "-INR 12,345,67.89", "-\u20b9 12,345,67.89",
+          "-INR 1234567.89", "-\u20b9 1234567.89"
+        }
+      }
+    },
+    {
+      "el_GR.UTF-8",
+      {
+        {
+          "1.234.567,89EUR", "1.234.567,89\u20ac",
+          "1234567,89EUR", "1234567,89\u20ac"
+        },
+        {
+          "-EUR1.234.567,89", "-\u20ac1.234.567,89",
+          "-EUR1234567,89", "-\u20ac1234567,89",
+        }
+      }
+    },
+    {}
+  };
+
+static int
+do_test (void)
+{
+  for (const struct locale_pair *test = tests;
+       test->locale_name != NULL; ++test)
+    {
+      init_loc (en_us_name, test->locale_name);
+      test_pair (&test->pair);
+      freelocale (loc);
+      test_en_us (test->locale_name);
+    }
+
+  return errors;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"

Re: [PATCH] strfmon_l: Use specified locale for number formatting [BZ #19633]

[Florian Weimer]

On 03/07/2016 09:05 PM, Florian Weimer wrote:
> On 03/07/2016 05:58 PM, Carlos O'Donell wrote:
>> On 03/07/2016 10:58 AM, Florian Weimer wrote:
>>> On 03/07/2016 04:28 PM, Carlos O'Donell wrote:
>>>
>>>> I tend to agree with Martin here, having that separation between
>>>> test and data makes it easier to read and change the test or add
>>>> more tests.
>>>>
>>>> If you're getting a warning from the compiler you expect but don't
>>>> care about then you can just silence the warning with the appropriate
>>>> attribute?
>>>
>>> The warning is not enabled by default (or even -W), so I'm not sure if
>>> that's even necessary.
>>
>> If the warning isn't enabled, then we don't need to worry about it today.
>> The vision here is that we can run everything with -Werror, but we aren't
>> there yet in some cases (see -Wundef fixes required).
> 
> Okay, here is a table-based version of the test.  Other parts of the
> patch are unchanged.

Ping?

  https://sourceware.org/ml/libc-alpha/2016-03/msg00176.html

Thanks,
Florian

Re: [PATCH] strfmon_l: Use specified locale for number formatting [BZ #19633]

[Martin Sebor]

On 03/30/2016 05:52 AM, Florian Weimer wrote:
> On 03/07/2016 09:05 PM, Florian Weimer wrote:
>> On 03/07/2016 05:58 PM, Carlos O'Donell wrote:
>>> On 03/07/2016 10:58 AM, Florian Weimer wrote:
>>>> On 03/07/2016 04:28 PM, Carlos O'Donell wrote:
>>>>
>>>>> I tend to agree with Martin here, having that separation between
>>>>> test and data makes it easier to read and change the test or add
>>>>> more tests.
>>>>>
>>>>> If you're getting a warning from the compiler you expect but don't
>>>>> care about then you can just silence the warning with the appropriate
>>>>> attribute?
>>>>
>>>> The warning is not enabled by default (or even -W), so I'm not sure if
>>>> that's even necessary.
>>>
>>> If the warning isn't enabled, then we don't need to worry about it today.
>>> The vision here is that we can run everything with -Werror, but we aren't
>>> there yet in some cases (see -Wundef fixes required).
>>
>> Okay, here is a table-based version of the test.  Other parts of the
>> patch are unchanged.
>
> Ping?
>
>    https://sourceware.org/ml/libc-alpha/2016-03/msg00176.html

I like it.

Martin

Re: [PATCH] strfmon_l: Use specified locale for number formatting [BZ #19633]

[Florian Weimer]

On 03/30/2016 05:08 PM, Martin Sebor wrote:
> On 03/30/2016 05:52 AM, Florian Weimer wrote:
>> On 03/07/2016 09:05 PM, Florian Weimer wrote:
>>> On 03/07/2016 05:58 PM, Carlos O'Donell wrote:
>>>> On 03/07/2016 10:58 AM, Florian Weimer wrote:
>>>>> On 03/07/2016 04:28 PM, Carlos O'Donell wrote:
>>>>>
>>>>>> I tend to agree with Martin here, having that separation between
>>>>>> test and data makes it easier to read and change the test or add
>>>>>> more tests.
>>>>>>
>>>>>> If you're getting a warning from the compiler you expect but don't
>>>>>> care about then you can just silence the warning with the appropriate
>>>>>> attribute?
>>>>>
>>>>> The warning is not enabled by default (or even -W), so I'm not sure if
>>>>> that's even necessary.
>>>>
>>>> If the warning isn't enabled, then we don't need to worry about it
>>>> today.
>>>> The vision here is that we can run everything with -Werror, but we
>>>> aren't
>>>> there yet in some cases (see -Wundef fixes required).
>>>
>>> Okay, here is a table-based version of the test.  Other parts of the
>>> patch are unchanged.
>>
>> Ping?
>>
>>    https://sourceware.org/ml/libc-alpha/2016-03/msg00176.html
> 
> I like it.

Thanks, committed.

Florian

Re: [PATCH] strfmon_l: Use specified locale for number formatting [BZ #19633]

[Stefan Liebler]

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

On 04/04/2016 04:20 PM, Florian Weimer wrote:
> On 03/30/2016 05:08 PM, Martin Sebor wrote:
>> On 03/30/2016 05:52 AM, Florian Weimer wrote:
>>> On 03/07/2016 09:05 PM, Florian Weimer wrote:
>>>> On 03/07/2016 05:58 PM, Carlos O'Donell wrote:
>>>>> On 03/07/2016 10:58 AM, Florian Weimer wrote:
>>>>>> On 03/07/2016 04:28 PM, Carlos O'Donell wrote:
>>>>>>
>>>>>>> I tend to agree with Martin here, having that separation between
>>>>>>> test and data makes it easier to read and change the test or add
>>>>>>> more tests.
>>>>>>>
>>>>>>> If you're getting a warning from the compiler you expect but don't
>>>>>>> care about then you can just silence the warning with the appropriate
>>>>>>> attribute?
>>>>>>
>>>>>> The warning is not enabled by default (or even -W), so I'm not sure if
>>>>>> that's even necessary.
>>>>>
>>>>> If the warning isn't enabled, then we don't need to worry about it
>>>>> today.
>>>>> The vision here is that we can run everything with -Werror, but we
>>>>> aren't
>>>>> there yet in some cases (see -Wundef fixes required).
>>>>
>>>> Okay, here is a table-based version of the test.  Other parts of the
>>>> patch are unchanged.
>>>
>>> Ping?
>>>
>>>     https://sourceware.org/ml/libc-alpha/2016-03/msg00176.html
>>
>> I like it.
>
> Thanks, committed.
>
> Florian
>
>

Hi,

on s390, i get elf/check-abi-libc.out:
--- ../sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist 
2016-03-04 14:08:24.443047376 +0100
+++ /home/stli/glibcDir/glibc-20160405-build/libc.symlist 
2016-04-05 08:34:18.215255347 +0200
@@ -2131,0 +2132 @@ GLIBC_2.4 __printf_fp F
+GLIBC_2.4 __printf_fp_l F


In stdio-common/printf_fp.c ldbl_-macros are used for __printf_fp_l ...:
ldbl_hidden_def (___printf_fp_l, __printf_fp_l)
ldbl_strong_alias (___printf_fp_l, __printf_fp_l)


... which are defined in sysdeps/generic/math_ldbl_opt.h
(e.g. used on x86_64):
#define ldbl_hidden_def(local, name) libc_hidden_def (name)
#define ldbl_strong_alias(name, aliasname) strong_alias (name, aliasname)


... or sysdeps/ieee754/ldbl-opt/math_ldbl_opt.h
(e.g used on s390):
#ifdef SHARED
# define ldbl_hidden_def(local, name) libc_hidden_ver (local, name)
# define ldbl_strong_alias(name, aliasname) \
   strong_alias (name, __GL_##name##_##aliasname) \
   long_double_symbol (libc, __GL_##name##_##aliasname, aliasname);


The attached patch uses the libc_hidden_def and strong_alias instead of 
ldbl_hidden_def and ldbl_strong_alias macros.

Then, the testsuite is clean on x86_64 and s390.

Please verify.

ChangeLog:

2016-04-05  Stefan Liebler  <stli@linux.vnet.ibm.com>

	* stdio-common/printf_fp.c (__printf_fp_l):
	Use libc_hidden_def and strong_alias instead of
	ldbl_hidden_def and ldbl_strong_alias macros.

[-- Attachment #2: 20160405_printf_fp_l.patch --]
[-- Type: text/x-patch, Size: 486 bytes --]

diff --git a/stdio-common/printf_fp.c b/stdio-common/printf_fp.c
index baada9e..fb2a763 100644
--- a/stdio-common/printf_fp.c
+++ b/stdio-common/printf_fp.c
@@ -1248,8 +1248,8 @@ ___printf_fp_l (FILE *fp, locale_t loc,
   }
   return done;
 }
-ldbl_hidden_def (___printf_fp_l, __printf_fp_l)
-ldbl_strong_alias (___printf_fp_l, __printf_fp_l)
+libc_hidden_def (__printf_fp_l)
+strong_alias (___printf_fp_l, __printf_fp_l)
 
 int
 ___printf_fp (FILE *fp, const struct printf_info *info,

Re: [PATCH] strfmon_l: Use specified locale for number formatting [BZ #19633]

[Florian Weimer]

On 04/05/2016 12:26 PM, Stefan Liebler wrote:

> on s390, i get elf/check-abi-libc.out:
> --- ../sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist 2016-03-04
> 14:08:24.443047376 +0100
> +++ /home/stli/glibcDir/glibc-20160405-build/libc.symlist 2016-04-05
> 08:34:18.215255347 +0200
> @@ -2131,0 +2132 @@ GLIBC_2.4 __printf_fp F
> +GLIBC_2.4 __printf_fp_l F
> 
> 
> In stdio-common/printf_fp.c ldbl_-macros are used for __printf_fp_l ...:
> ldbl_hidden_def (___printf_fp_l, __printf_fp_l)
> ldbl_strong_alias (___printf_fp_l, __printf_fp_l)
> 
> 
> ... which are defined in sysdeps/generic/math_ldbl_opt.h
> (e.g. used on x86_64):
> #define ldbl_hidden_def(local, name) libc_hidden_def (name)
> #define ldbl_strong_alias(name, aliasname) strong_alias (name, aliasname)
> 
> 
> ... or sysdeps/ieee754/ldbl-opt/math_ldbl_opt.h
> (e.g used on s390):
> #ifdef SHARED
> # define ldbl_hidden_def(local, name) libc_hidden_ver (local, name)
> # define ldbl_strong_alias(name, aliasname) \
>   strong_alias (name, __GL_##name##_##aliasname) \
>   long_double_symbol (libc, __GL_##name##_##aliasname, aliasname);

Sorry about that.  I asked for guidance on this aspect earlier, but did
not receive any response.

The main question here is if we need additional symbols because we have
a dependency on the double layout, and the code needs to be compiled
twice.  I assume this is the point of the ldbl mechanism, but I'm not sure.

Florian

Re: [PATCH] strfmon_l: Use specified locale for number formatting [BZ #19633]

[Andreas Schwab]

sysdeps/ieee754/ldbl-opt needs to be updated to provide
__nldbl___printf_fp_l for -mlong-double-64.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] strfmon_l: Use specified locale for number formatting [BZ #19633]

[Andreas Schwab]

Please disregard, Stefan's patch is correct.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] strfmon_l: Use specified locale for number formatting [BZ #19633]

[Andreas Schwab]

Stefan Liebler <stli@linux.vnet.ibm.com> writes:

> diff --git a/stdio-common/printf_fp.c b/stdio-common/printf_fp.c
> index baada9e..fb2a763 100644
> --- a/stdio-common/printf_fp.c
> +++ b/stdio-common/printf_fp.c
> @@ -1248,8 +1248,8 @@ ___printf_fp_l (FILE *fp, locale_t loc,
>    }
>    return done;
>  }
> -ldbl_hidden_def (___printf_fp_l, __printf_fp_l)
> -ldbl_strong_alias (___printf_fp_l, __printf_fp_l)
> +libc_hidden_def (__printf_fp_l)
> +strong_alias (___printf_fp_l, __printf_fp_l)

Please rename ___printf_fp_l to __printf_fp_l, no need for the alias.
Otherwise looks ok.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] strfmon_l: Use specified locale for number formatting [BZ #19633]

[Stefan Liebler]

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

On 04/05/2016 02:50 PM, Andreas Schwab wrote:
> Stefan Liebler <stli@linux.vnet.ibm.com> writes:
>
>> diff --git a/stdio-common/printf_fp.c b/stdio-common/printf_fp.c
>> index baada9e..fb2a763 100644
>> --- a/stdio-common/printf_fp.c
>> +++ b/stdio-common/printf_fp.c
>> @@ -1248,8 +1248,8 @@ ___printf_fp_l (FILE *fp, locale_t loc,
>>     }
>>     return done;
>>   }
>> -ldbl_hidden_def (___printf_fp_l, __printf_fp_l)
>> -ldbl_strong_alias (___printf_fp_l, __printf_fp_l)
>> +libc_hidden_def (__printf_fp_l)
>> +strong_alias (___printf_fp_l, __printf_fp_l)
>
> Please rename ___printf_fp_l to __printf_fp_l, no need for the alias.
> Otherwise looks ok.
>
> Andreas.
>
Here is an updated patch to reflect the mentioned changes.
Okay to commit?

ChangeLog:

2016-04-05  Stefan Liebler  <stli@linux.vnet.ibm.com>

	* stdio-common/printf_fp.c (__printf_fp_l):
	Rename ___printf_fp_l to __printf_fp_l and
	remove strong alias. Use libc_hidden_def instead
	of ldbl_hidden_def macro.

[-- Attachment #2: 20160405_printf_fp_l_2.patch --]
[-- Type: text/x-patch, Size: 1045 bytes --]

diff --git a/stdio-common/printf_fp.c b/stdio-common/printf_fp.c
index baada9e..fdfe06b 100644
--- a/stdio-common/printf_fp.c
+++ b/stdio-common/printf_fp.c
@@ -209,9 +209,9 @@ hack_digit (struct hack_digit_param *p)
 }
 
 int
-___printf_fp_l (FILE *fp, locale_t loc,
-		const struct printf_info *info,
-		const void *const *args)
+__printf_fp_l (FILE *fp, locale_t loc,
+	       const struct printf_info *info,
+	       const void *const *args)
 {
   /* The floating-point value to output.  */
   union
@@ -1248,14 +1248,13 @@ ___printf_fp_l (FILE *fp, locale_t loc,
   }
   return done;
 }
-ldbl_hidden_def (___printf_fp_l, __printf_fp_l)
-ldbl_strong_alias (___printf_fp_l, __printf_fp_l)
+libc_hidden_def (__printf_fp_l)
 
 int
 ___printf_fp (FILE *fp, const struct printf_info *info,
 	      const void *const *args)
 {
-  return ___printf_fp_l (fp, _NL_CURRENT_LOCALE, info, args);
+  return __printf_fp_l (fp, _NL_CURRENT_LOCALE, info, args);
 }
 ldbl_hidden_def (___printf_fp, __printf_fp)
 ldbl_strong_alias (___printf_fp, __printf_fp)

Re: [PATCH] strfmon_l: Use specified locale for number formatting [BZ #19633]

[Stefan Liebler]

@Andreas:
Just to be sure. Is this second patch okay? Then I`ll commit it.

Bye Stefan

On 04/05/2016 05:05 PM, Stefan Liebler wrote:
> On 04/05/2016 02:50 PM, Andreas Schwab wrote:
>> Stefan Liebler <stli@linux.vnet.ibm.com> writes:
>>
>>> diff --git a/stdio-common/printf_fp.c b/stdio-common/printf_fp.c
>>> index baada9e..fb2a763 100644
>>> --- a/stdio-common/printf_fp.c
>>> +++ b/stdio-common/printf_fp.c
>>> @@ -1248,8 +1248,8 @@ ___printf_fp_l (FILE *fp, locale_t loc,
>>>     }
>>>     return done;
>>>   }
>>> -ldbl_hidden_def (___printf_fp_l, __printf_fp_l)
>>> -ldbl_strong_alias (___printf_fp_l, __printf_fp_l)
>>> +libc_hidden_def (__printf_fp_l)
>>> +strong_alias (___printf_fp_l, __printf_fp_l)
>>
>> Please rename ___printf_fp_l to __printf_fp_l, no need for the alias.
>> Otherwise looks ok.
>>
>> Andreas.
>>
> Here is an updated patch to reflect the mentioned changes.
> Okay to commit?
>
> ChangeLog:
>
> 2016-04-05  Stefan Liebler  <stli@linux.vnet.ibm.com>
>
>      * stdio-common/printf_fp.c (__printf_fp_l):
>      Rename ___printf_fp_l to __printf_fp_l and
>      remove strong alias. Use libc_hidden_def instead
>      of ldbl_hidden_def macro.

Re: [PATCH] strfmon_l: Use specified locale for number formatting [BZ #19633]

[Stefan Liebler]

On 04/05/2016 05:05 PM, Stefan Liebler wrote:
> On 04/05/2016 02:50 PM, Andreas Schwab wrote:
>> Stefan Liebler <stli@linux.vnet.ibm.com> writes:
>>
>>> diff --git a/stdio-common/printf_fp.c b/stdio-common/printf_fp.c
>>> index baada9e..fb2a763 100644
>>> --- a/stdio-common/printf_fp.c
>>> +++ b/stdio-common/printf_fp.c
>>> @@ -1248,8 +1248,8 @@ ___printf_fp_l (FILE *fp, locale_t loc,
>>>     }
>>>     return done;
>>>   }
>>> -ldbl_hidden_def (___printf_fp_l, __printf_fp_l)
>>> -ldbl_strong_alias (___printf_fp_l, __printf_fp_l)
>>> +libc_hidden_def (__printf_fp_l)
>>> +strong_alias (___printf_fp_l, __printf_fp_l)
>>
>> Please rename ___printf_fp_l to __printf_fp_l, no need for the alias.
>> Otherwise looks ok.
>>
>> Andreas.
>>
> Here is an updated patch to reflect the mentioned changes.
> Okay to commit?
>
> ChangeLog:
>
> 2016-04-05  Stefan Liebler  <stli@linux.vnet.ibm.com>
>
>      * stdio-common/printf_fp.c (__printf_fp_l):
>      Rename ___printf_fp_l to __printf_fp_l and
>      remove strong alias. Use libc_hidden_def instead
>      of ldbl_hidden_def macro.
Commited