Re: [PATCH v11 1/5] Implement alternative month names (bug 10871).

[Rafal Luzynski <digitalfreak at lingonborough dot com>]

11.01.2018 03:16 "Dmitry V. Levin" <ldv@altlinux.org> wrote:
>
>
> On Tue, Jan 09, 2018 at 12:59:21AM +0100, Rafal Luzynski wrote:
> > [BZ #10871]
> > * locale/C-time.c: Add alternative month names, define them as the
> > same as mon explicitly.
>
> * locale/C-time.c (_nl_C_LC_TIME): Add alternative month names.
>
> I don't understand what do you mean by "define them as the same as mon
> explicitly", though.

Initially I had an idea to add the alternative month names but
initialize them all to empty strings ("") and rely on the mechanism
which falls back to the regular month names if the alternative ones
are empty.  Then this has been changed to initialize them just "January",
"February", etc.  How to name those month names?  Regular?  Nominative?
Basic?  Non-alternative?

> [...]
> > * locale/programs/ld-time.c: Alternative month names support
> > added, they are a copy of mon if not specified explicitly.
>
> * locale/programs/ld-time.c (struct locale_time_t): Add alt_mon,
> walt_mon, and alt_mon_defined members.
> (time_output): Output alt_mon and walt_mon members.
> (time_read): Read them, initialize alt_mon_defined.

This is OK but shouldn't we mention that alt_mon content is copied
from mon (and walt_mon from wmon) if not specified explicitly in the
locale data file?

> [...]
> > * time/strptime_l.c: Alternative month names also recognized.
>
> * time/strptime_l.c (alt_month_name): New macro.
> (__strptime_internal) [_LIBC]: Add ... (whatever is being added).
> Handle %OB format.

I'm confused.  Actually alt_month_name has been added globally
in the #ifdef _LIBC section so I think this makes sense:

* time/strptime_l.c [_LIBC] (alt_month_name): New macro.

But I don't understand what do you mean by "(__strptime_internal)
[_LIBC]: Add ... (whatever is being added)."  It's "Handle %OB format"
what has been added.  And it's not really conditional [_LIBC]; the
difference is that if _LIBC is not defined then %OB format is accepted
but alternative month names are not checked (because alt_month_name does
not exist).  Is this a copy-paste typo?  What else do I miss?

> [...]
> > --- a/locale/langinfo.h
> > +++ b/locale/langinfo.h
> > @@ -100,7 +100,8 @@ enum
> > ABMON_12,
> > #define ABMON_12 ABMON_12
> >
> > - /* Long month names. */
> > + /* Long month names, in the grammatical form used when the month
> > + forms part of a complete date. */
>
> ... when month name is a part of ...?

Does the question mark mean that you are not sure?  Well, this comment
has been provided by Zack Weinberg who is a native English speaker.
We both are not native speakers so I'd rather trust Zack here.  So far
I don't change this comment unless other native speakers tell me to change.

>
> --
> ldv

Thank you Dmitry for your review.  I fully agree with your other comments
which I don't quote here and I'm applying them locally.

Regards,

Rafal