This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v11 1/5] Implement alternative month names (bug 10871).
On 01/11/2018 07:44 PM, Rafal Luzynski wrote:
> Thank you Carlos for your reviews. Please find my comments below:
>
> 11.01.2018 06:03 Carlos O'Donell <carlos@redhat.com> wrote:
>>
>> On 01/08/2018 03:59 PM, Rafal Luzynski wrote:
>>> [...]
>>> +#ifdef __USE_GNU
>>> +# define ALTMON_1 __ALTMON_1
>>> +# define ALTMON_2 __ALTMON_2
>>> +# define ALTMON_3 __ALTMON_3
>>> +# define ALTMON_4 __ALTMON_4
>>> +# define ALTMON_5 __ALTMON_5
>>> +# define ALTMON_6 __ALTMON_6
>>> +# define ALTMON_7 __ALTMON_7
>>> +# define ALTMON_8 __ALTMON_8
>>> +# define ALTMON_9 __ALTMON_9
>>> +# define ALTMON_10 __ALTMON_10
>>> +# define ALTMON_11 __ALTMON_11
>>> +# define ALTMON_12 __ALTMON_12
>>
>> OK. Requires _GNU_SOURCE, which is good because this is an extension not
>> yet defined by POSIX.
>
> I guess you meant __USE_GNU because that's what my patch uses. So does
> /usr/include/langinfo.h everywhere. Should I use _GNU_SOURCE instead?
Your use of __USE_GNU is *correct*, that is the internal macro that glibc
uses when enabling GNU extensions.
The user or developer will not use __USE_GNU, instead they will define
_GNU_SOURCE (feature test macro), and that will under the hood define
__USE_GNU (see include/features.h).
Again, you don't need to change anything here, my "OK." indicates that
I saw everything correct. My subsequent comment is designed to educate
those reading the review and provide insight into what I was looking
for in my review.
>>> [...]
>>> diff --git a/time/tst-strptime.c b/time/tst-strptime.c
>>> index 34ad797..bbc1390 100644
>>> --- a/time/tst-strptime.c
>>> +++ b/time/tst-strptime.c
>>> @@ -51,6 +51,12 @@ static const struct
>>> 6, 0, 0, 1 },
>>> { "ja_JP.EUC-JP", "2001 20 \xb7\xee", "%Y %U %a", 1, 140, 4, 21 },
>>> { "ja_JP.EUC-JP", "2001 21 \xb7\xee", "%Y %W %a", 1, 140, 4, 21 },
>>> + { "pl_PL.UTF-8", "21 lis 2017", "%d %b %Y", 2, 324, 10, 21 },
>>> + { "pl_PL.UTF-8", "22 LIS 2017", "%d %B %Y", 3, 325, 10, 22 },
>>> + /* TODO: Use the genitive case here as soon as it is added to localedata.
>>> */
>>> + { "pl_PL.UTF-8", "23 listopad 2017", "%d %B %Y", 4, 326, 10, 23 },
>>> + /* The nominative case is incorrect here but it is parseable. */
>>> + { "pl_PL.UTF-8", "24 listopad 2017", "%d %OB %Y", 5, 327, 10, 24 },
>>
>> Here we need examples of %OB for langauges that do *not* have genitive names
>> in order to test the conversion of %OB for locales that would have this
>> definition missing (tests that %OB does fall back to %B).
>
> OK, I'm adding en_US.ISO-8859-1 for %B, de_DE.ISO-8859-1 for %b
> (those locales look weird but they are already used in this file so
> we don't need to add them), fr_FR.UTF-8 for %OB, and then I'll also
> add es_ES.UTF-8 for %Ob in another patch which adds %Ob.
Perfect. Thank you very much.
--
Cheers,
Carlos.