Re: [PATCH] localedef: Add --no-warnings/--warnings option

[Florian Weimer <fweimer at redhat dot com>]

On 10/17/2017 10:59 AM, Carlos O'Donell wrote:

>         if (failed)
>   	{
> -	  record_warning (_("\
> -character map `%s' is not ASCII compatible, locale not ISO C compliant\n"),
> -			  result->code_set_name);
> +	  /* A user may disable the ASCII compatibility warning check,
> +	     but we must remember that the encoding is not ASCII
> +	     compatible, since it may have other implications.  Later
> +	     we will set _NL_CTYPE_MAP_TO_NONASCII from this value.  */
> +	  if (warn_ascii)
> +	    record_warning (_(
> +"character map `%s' is not ASCII compatible, locale not ISO C compliant "
> +"[--no-warnings=ascii]"),
> +			    result->code_set_name);
>   	  enc_not_ascii_compatible = true;
>   	}

It may lead to generally nicer code if the flag check happens in 
record_warning itself.

> diff --git a/locale/programs/charmap.h b/locale/programs/charmap.h
> index 5d6b48f..441d429 100644
> --- a/locale/programs/charmap.h
> +++ b/locale/programs/charmap.h
> @@ -66,6 +66,8 @@ struct charseq
>   
>   /* True if the encoding is not ASCII compatible.  */
>   extern bool enc_not_ascii_compatible;
> +/* True if the ASCII compatibility check should raise a warning.  */
> +bool warn_ascii;

You have a non-tentative definition elsewhere, so this should indeed be 
extern.

>   /* Prototypes for charmap handling functions.  */
> diff --git a/locale/programs/ld-monetary.c b/locale/programs/ld-monetary.c
> index 9d94738..71df376 100644
> --- a/locale/programs/ld-monetary.c
> +++ b/locale/programs/ld-monetary.c
> @@ -234,12 +234,17 @@ No definition for %s category found"), "LC_MONETARY");
>   	  char symbol[4];
>   	  strncpy (symbol, monetary->int_curr_symbol, 3);
>   	  symbol[3] = '\0';
> +	  /* A user may disable this waning for testing purposes or
> +	     for building a locale with a 3 digit country code that

“3 letter country code”

> +	     was not yet supported in our ISO 4217 list.
> +	     See the use of --no-warnings=intcurrsym.  */
>   	  if (bsearch (symbol, valid_int_curr, NR_VALID_INT_CURR,
>   		       sizeof (const char *),
> -		       (comparison_fn_t) curr_strcmp) == NULL)
> +		       (comparison_fn_t) curr_strcmp) == NULL
> +	      && warn_int_curr_symbol)
>   	    record_warning (_("\
>   %s: value of field `int_curr_symbol' does \
> -not correspond to a valid name in ISO 4217"),
> +not correspond to a valid name in ISO 4217 [--no-warnings=intcurrsym]"),
>   			    "LC_MONETARY");

> +static void
> +set_warnings (char *warnings, bool enabled)
> +{
> +  char *tok;
> +  char *save;
> +  char *copy = (char *) malloc (strlen (warnings) + 1);
> +
> +  /* Remove all spaces from the warnings list to make the processing
> +     a more robust.  We don't support spaces in a warning name.  */
> +
> +  save = copy;
> +  tok = warnings;

Style: Declare on first use?  (I think that's preferred nowadays.)

> +
> +  do {
> +    while (isspace (*tok))
> +      tok++;
> +  } while ((*save++ = *tok++));

{ } need to follow GNU style, and the comparison against '\0' should be 
explicit.

The other parts of the patch look fine to me.

> +# The SHIFT_JIS and SHIFT_JISX0213 character maps are not ASCII compatible,
> +# therefore we have to use --no-warnings=ascii to disable the ASCII check.
> +# See localedata/gen-locale.sh for the same logic.
>   $(INSTALL-SUPPORTED-LOCALES): install-locales-dir
>   	@locale=`echo $@ | sed -e 's/^install-//'`; \
>   	charset=`echo $$locale | sed -e 's,.*/,,'`; \
>   	locale=`echo $$locale | sed -e 's,/[^/]*,,'`; \
> +	flags="--quiet -c"; \
> +	if [ "$$charset" = 'SHIFT_JIS' ] \
> +	   || [ "$$charset" = 'SHIFT_JISX0213' ]; then \
> +	   flags="$$flags --no-warnings=ascii"; \
> +	fi; \
>   	echo -n `echo $$locale | sed 's/\([^.\@]*\).*/\1/'`; \
>   	echo -n ".$$charset"; \
>   	echo -n `echo $$locale | sed 's/\([^\@]*\)\(\@.*\)*/\2/'`; \
>   	echo -n '...'; \
>   	input=`echo $$locale | sed 's/\([^.]*\)[^@]*\(.*\)/\1\2/'`; \
> -	$(LOCALEDEF) --alias-file=../intl/locale.alias \
> -		     -i locales/$$input -c -f charmaps/$$charset \
> +	$(LOCALEDEF) $$flags --alias-file=../intl/locale.alias \
> +		     -i locales/$$input -f charmaps/$$charset \
>   		     $(addprefix --prefix=,$(install_root)) $$locale \
>   	&& echo ' done'; \

Maybe it's time to move this recipe into its own file under scripts/?

Thanks,
Florian