diff mbox

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

Message ID 56DDDF0D.8040601@redhat.com
State New
Headers show

Commit Message

Florian Weimer March 7, 2016, 8:05 p.m. UTC
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

Comments

Florian Weimer March 30, 2016, 11:52 a.m. UTC | #1
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
Martin Sebor March 30, 2016, 3:08 p.m. UTC | #2
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
Florian Weimer April 4, 2016, 2:20 p.m. UTC | #3
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
diff mbox

Patch

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)
+
 
 /* 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"