This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] [BZ 17588 13064] Update UTF-8 charmap and width to Unicode 7.0.0


On 02/12/2015 05:18 AM, Alexandre Oliva wrote:
>> Regression tested on x86_64-linux-gnu.  Ok to install?

Yes, this version is OK to install if you fix all the nits.

Despite complaints that a change in the generator would create
a smaller diff, that doesn't matter to me. You can still review
the output and it is readable (and I used scripts to review the
automatically generated code).

This is a wonderful step up, being able to do `make`, `make check`,
and `make install` from the generators directory is amazing automation!

Way to go team! :-)

>> Unicode 7.0.0 update; added generator scripts.
> 
>> From: Alexandre Oliva <aoliva@redhat.com>
> 
>> for  localedata/ChangeLog
> 

Nit: ChangeLog needs [BZ #xxx] etc.

Nit: This covers bugs 17588, 13064, *AND* 14094.

Nit: Needs a NEWS entry describing this in full glory :-)

>> 	* unicode-gen/Makefile: New.

OK.

Some might argue it fits better under "scripts" e.g. scripts/unicode-gen,
but I don't care. We can move it later if we think it should move at all.

>> 	* unicode-gen/gen_unicode_ctype.py: New generator.

Nit: Wrong copyright year e.g. 2014 -> 2015.

Nit: We don't use "Contributed by" statements, they are instead pat of what
     git records as Author or in the git commit message.

     https://sourceware.org/glibc/wiki/Contribution%20checklist#Attribution

     This nit applies to all the files that have "Contributed by"

OK.

>> 	* unicode-gen/ctype_compatibility.py: New verifier.

OK.

>> 	* unicode-gen/ctype_compatibility_test_cases.py: New verifier
>> 	module.

OK.

>> 	* unicode-gen/utf8_gen.py: New generator.

OK.

>> 	* unicode-gen/utf8_compatibility: New verifier.

OK.

>> 	* charmaps/UTF-8: Update.

OK.

>> 	* locales/i18n: Update.

OK. 

Note: Manually verified certain conversions to make sure they matched.
      Manual verification passed OK.

>> 	* gen-unicode-ctype.c: Remove.

OK.

>> 	* tst-ctype-de_DE.ISO-8859-1.in: Adjust, islower now returns
>> 	true for ordinal indicators.

Nit: This need a specific new BZ for the fix to user-visible behaviour.
     Discussed in https://sourceware.org/bugzilla/show_bug.cgi?id=14094#c34,
     but should have a bug of it's own with a comment about why the old
     behaviour is wrong, and that the existing regression test covers the
     change and has been fixed.

Cheers,
Carlos.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]