Re: [RFC][PATCH v8 01/16] Implement alternative month names (bug 10871).

[Rafal Luzynski <digitalfreak at lingonborough dot com>]

3.07.2017 23:20 Zack Weinberg <zackw@panix.com> wrote:
>
> On 06/28/2017 05:58 AM, Rafal Luzynski wrote:
> > [...]
> > --- a/locale/C-time.c
> > +++ b/locale/C-time.c
>
> Is it really correct to define the new strings as empty strings for the
> "C" locale? Shouldn't they be equal to the MON_* strings?

It's probably not.  This must be a leftover which was meant to be
overwritten by part 8 of this patch series which makes altmon array
a copy of mon array by default.

>
> > --- a/locale/langinfo.h
> > +++ b/locale/langinfo.h
> > [...]
> > + /* Some languages need both standalone and a full-date formatting form,
> > + usually nominative and genitive. However, it is not yet decided
> > + whether the alternative form is standalone (nominative) and the
> > + regular one is full-date formatting (genitive) or vice versa.
> > + Languages which do not need nominative and genitive month names
> > + can ignore this feature. */
>
> I think hedging this may be a hangover from my old suggestion that we
> avoid making a decision just yet -- that was me trying to break the
> deadlock and get %OB support into 2.25; that ship has sailed.

That's correct.

> Let's go ahead and follow POSIX.

This has always been my point.

> I also think we should avoid referring to specific cases in these
> comments. So my suggested wordings are
>
> + /* Long month names, in the grammatical form used when the month
> + forms part of a complete date. */
>
> for MON_*, and
>
> + /* Long month names, in the grammatical form used when the month
> + is named by itself. */
>
> for ALTMON_*.

I agree, we already went through the discussion that the words
"nominative" and "genitive" may be misleading because some languages
use different set of cases (e.g., always nominative).

> > --- a/locale/loadlocale.c
> > +++ b/locale/loadlocale.c
> > @@ -45,7 +45,8 @@ static const size_t _nl_category_num_items[] =
> > #define NO_PAREN(arg, rest...) arg, ##rest
> >
> > #define DEFINE_CATEGORY(category, category_name, items, a) \
> > -static const enum value_type _nl_value_type_##category[] = { NO_PAREN items
> > };
> > +static const enum value_type _nl_value_type_##category \
> > + [_NL_ITEM_INDEX (_NL_NUM_##category)] = { NO_PAREN items };
>
> Why is this change necessary? The empty array brackets should make the
> compiler do the Right Thing.

It's a little longer story and indeed kinda off-topic so I will
provide some links.  Shortly summarizing: the empty array brackets
does not make the compiler do the Right Thing if the last element
is a subarray itself like mon or altmon.  In such case the last item
marks the beginning of the subarray and makes the compiler skip all
elements of the subarray except the first one.  Links:

https://sourceware.org/ml/libc-alpha/2016-03/msg00557.html
https://sourceware.org/ml/libc-alpha/2016-03/msg00521.html [patch]
https://sourceware.org/bugzilla/show_bug.cgi?id=19084 [patch]

> If this change is genuinely necessary for some reason, it should be
> posted, reviewed, and committed independently. (It's OK if it only
> becomes necessary with the rest of these patches, you just have to
> explain that in the commit message.

It can be interpreted both ways.  This is indeed a separate bug
but will not matter until the altmon array is added.  If you
prefer it to be posted separately then it has been posted already,
see the links above.

> > --- a/locale/programs/locfile-kw.h
> > +++ b/locale/programs/locfile-kw.h
>
> Please don't include the diffs for generated-but-checked-in files in
> posts to libc-alpha.

I'm not sure I understand this correctly.  Should I assume that
the committer will regenerate this file and commit it even if my
patch does not include the change?

> ----
>
> This review probably sounds terribly nit-picky; that is because the
> patch, overall, is solid, it just needs a bit of polish.

Thank you, Zack.  Your review is very valuable for me and for everyone
who will ever profit from this bugfix.

Regards,

Rafal