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

Carlos O'Donell carlos@redhat.com
Tue Oct 17 22:07:00 GMT 2017 

On 10/17/2017 02:52 AM, Florian Weimer wrote:
> 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.

I considered it, but it would require richer API for the
warnings being recorded, so for now I'm going to leave this
as-is.

Though I agree with you that it is nicer if the caller simply
filled in a structure, and submitted the warning.

Then the warning code handled deciding of the warning was enabled
or not. I'm not keen to refactor this code all over again just to
support that kind of use case, mainly because we might not need it.

If I moved this code into support/ I would do that kind of refactoring.

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

I do. In localedef. There is a use of charmap.o in iconv (as I mentioned to
Andreas). I'm going to move record-status.h to record-status.c and centralize
the warning handling variables. That should clean this up.

>>   /* 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”

Fixed. Thanks.

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

I cleaned this up.

Basically we use tok/save/copy right away in the space removal loop.

>> +
>> +  do {
>> +    while (isspace (*tok))
>> +      tok++;
>> +  } while ((*save++ = *tok++));
> 
> { } need to follow GNU style, and the comparison against '\0' should be explicit.

Fixed.

> 
> 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/?

I don't want to tackle that in this patch. Though I agree, this should get
deleted and localedata/gen-locale.sh should become the canonical script
we use to install locales. However it requires refacoring gen-locales.sh
a bit to be generally usable for installing test locales and installing
final locales.

Please find v2 attached, no regressions.

In v2 I have moved all warning related variables into record-status.c,
and adjusted iconv, locale, and localedef to use that and avoid
duplication.

OK to commit?

-- 
Cheers,
Carlos.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-localedef-Add-no-warnings-warnings-option.patch
Type: text/x-patch
Size: 29179 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20171017/a132371c/attachment.bin>

More information about the Libc-alpha mailing list