Re: [PATCH v2 1/2] localedef: Update LC_MONETARY handling (Bug 28845)

[DJ Delorie <dj@redhat.com>]

Two comment changes but otherwise OK.

Reviewed-by: DJ Delorie <dj@redhat.com>

"Carlos O'Donell" <carlos@redhat.com> writes:
> diff --git a/locale/programs/ld-monetary.c b/locale/programs/ld-monetary.c

> +	mon_grouping		"\177" i.e. terminating -1

\177 is char *max*, not -1.  \377 is -1.

> +	mon_grouping		"\177" i.e. terminating -1

Likewise.

> +  /* The purpose of TEST_ELEM is to define a default value for the fields
> +     in the category if the field was not defined in the cateory.  If the
> +     category was present but we didn't see a definition for the field then
> +     we also issue a warning, otherwise the only warning you get is the one
> +     earlier when a default category is created (completely missing category).
> +     This missing field warning is glibc-specific since no standard requires
> +     this warning, but we consider it valuable to print a warning for all
> +     missing fields in the category.  */

Ok.

>  #define TEST_ELEM(cat, initval) \
>    if (monetary->cat == NULL)						      \
>      {									      \
>        if (! nothing)							      \
> -	record_error (0, 0, _("%s: field `%s' not defined"),		      \
> -		      "LC_MONETARY", #cat);				      \
> +	record_warning (_("%s: field `%s' not defined"),		      \
> +			"LC_MONETARY", #cat);				      \

Ok.

> +  /* Keyword: int_curr_symbol.  */
>    TEST_ELEM (int_curr_symbol, "");
> -  TEST_ELEM (currency_symbol, "");
> -  TEST_ELEM (mon_thousands_sep, "");
> -  TEST_ELEM (positive_sign, "");
> -  TEST_ELEM (negative_sign, "");

Ok.

> @@ -247,41 +331,63 @@ not correspond to a valid name in ISO 4217 [--no-warnings=intcurrsym]"),
>  	}
>      }
>  
> -  /* The decimal point must not be empty.  This is not said explicitly
> -     in POSIX but ANSI C (ISO/IEC 9899) says in 4.4.2.1 it has to be
> -     != "".  */
> +  /* Keyword: currency_symbol */
> +  TEST_ELEM (currency_symbol, "");
> +
> +  /* Keyword: mon_decimal_point */
> +  /* ISO C17 7.11.2.1.3 explicitly allows mon_decimal_point to be the
> +     empty string e.g. "".  This indicates the value is not available in the
> +     current locale or is of zero length.  However, if the value was never
> +     defined then we issue a warning and use a glibc-specific default.  ISO
> +     30112 in the i18n FDCC-Set uses <U002C> ",", and POSIX Issue 7 in the
> +     POSIX locale uses "".  It is specific to glibc that the default is <U002E>
> +     "."; we retain this existing behaviour for backwards compatibility.  */

Ok.

>    if (monetary->mon_decimal_point == NULL)
>      {
>        if (! nothing)
> -	record_error (0, 0, _("%s: field `%s' not defined"),
> -		      "LC_MONETARY", "mon_decimal_point");
> +	record_warning (_("%s: field `%s' not defined, using defaults"),
> +			"LC_MONETARY", "mon_decimal_point");

Ok.

> -  else if (monetary->mon_decimal_point[0] == '\0' && ! be_quiet && ! nothing)
> -      record_error (0, 0, _("\
> -%s: value for field `%s' must not be an empty string"),
> -		    "LC_MONETARY", "mon_decimal_point");

Ok.

> +
> +  /* Keyword: mon_thousands_sep */
> +  if (monetary->mon_thousands_sep == NULL)
>      {
> +      if (! nothing)
> +	record_warning (_("%s: field `%s' not defined, using defaults"),
> +			"LC_MONETARY", "mon_thousands_sep");
> +      monetary->mon_thousands_sep = "";
> +      monetary->mon_thousands_sep_wc = L'\0';
>      }

Ok.

> +  /* Keyword: mon_grouping */
>    if (monetary->mon_grouping_len == 0)
>      {
>        if (! nothing)
> -	record_error (0, 0, _("%s: field `%s' not defined"),
> -		      "LC_MONETARY", "mon_grouping");
> -
> +	record_warning (_("%s: field `%s' not defined"),
> +			"LC_MONETARY", "mon_grouping");

Ok.

> +      /* Missing entries are given 1 element in their bytearray with
> +	 a value of CHAR_MAX which indicates that "No further grouping
> +	 is to be performed" (functionally equivalent to ISO C's "C"
> +	 locale default of ""). */
>        monetary->mon_grouping = (char *) "\177";
>        monetary->mon_grouping_len = 1;

Ok.

> +  /* Keyword: positive_sign */
> +  TEST_ELEM (positive_sign, "");
> +
> +  /* Keyword: negative_sign */
> +  TEST_ELEM (negative_sign, "");
> +

Ok.

>  #undef TEST_ELEM
>  #define TEST_ELEM(cat, min, max, initval) \
>    if (monetary->cat == -2)						      \
>      {									      \
>         if (! nothing)							      \
> -	 record_error (0, 0, _("%s: field `%s' not defined"),		      \
> -		       "LC_MONETARY", #cat);				      \
> +	 record_warning (_("%s: field `%s' not defined"),		      \
> +			 "LC_MONETARY", #cat);				      \

Ok.

> @@ -300,16 +406,11 @@ not correspond to a valid name in ISO 4217 [--no-warnings=intcurrsym]"),
>    TEST_ELEM (p_sign_posn, -1, 4, -1);
>    TEST_ELEM (n_sign_posn, -1, 4, -1);
>  
> -  /* The non-POSIX.2 extensions are optional.  */
> -  if (monetary->duo_int_curr_symbol == NULL)
> -    monetary->duo_int_curr_symbol = monetary->int_curr_symbol;
> -  if (monetary->duo_currency_symbol == NULL)
> -    monetary->duo_currency_symbol = monetary->currency_symbol;
> -
> -  if (monetary->duo_int_frac_digits == -2)
> -    monetary->duo_int_frac_digits = monetary->int_frac_digits;
> -  if (monetary->duo_frac_digits == -2)
> -    monetary->duo_frac_digits = monetary->frac_digits;

Ok.

> +  /* Keyword: crncystr */
> +  monetary->crncystr = (char *) xmalloc (strlen (monetary->currency_symbol)
> +					 + 2);
> +  monetary->crncystr[0] = monetary->p_cs_precedes ? '-' : '+';
> +  strcpy (&monetary->crncystr[1], monetary->currency_symbol);

Ok.

> @@ -327,6 +428,17 @@ not correspond to a valid name in ISO 4217 [--no-warnings=intcurrsym]"),
>    TEST_ELEM (int_p_sign_posn, p_sign_posn, -1, 4);
>    TEST_ELEM (int_n_sign_posn, n_sign_posn, -1, 4);
>  
> +  /* The non-POSIX.2 extensions are optional.  */
> +  if (monetary->duo_int_curr_symbol == NULL)
> +    monetary->duo_int_curr_symbol = monetary->int_curr_symbol;
> +  if (monetary->duo_currency_symbol == NULL)
> +    monetary->duo_currency_symbol = monetary->currency_symbol;
> +
> +  if (monetary->duo_int_frac_digits == -2)
> +    monetary->duo_int_frac_digits = monetary->int_frac_digits;
> +  if (monetary->duo_frac_digits == -2)
> +    monetary->duo_frac_digits = monetary->frac_digits;
> +

Ok.

> +  /* Keyword: conversion_rate */

Ok.
>  
> -  /* Create the crncystr entry.  */
> -  monetary->crncystr = (char *) xmalloc (strlen (monetary->currency_symbol)
> -					 + 2);
> -  monetary->crncystr[0] = monetary->p_cs_precedes ? '-' : '+';
> -  strcpy (&monetary->crncystr[1], monetary->currency_symbol);
> +  /* A value for monetary-decimal-point-wc was set when
> +     monetary_decimal_point was set, likewise for monetary-thousands-sep-wc.  */
>  }

Ok.