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

[Zack Weinberg <zackw at panix dot com>]

On 06/28/2017 05:58 AM, Rafal Luzynski wrote:

> [BZ #10871]
> * locale/C-time.c: define all alternative month names as empty strings.
> * locale/categories.def: alt_mon and wide-alt_mon added.
> * locale/langinfo.h: ALTMON_1 .. ALTMON_12 and similar contants defined.
> * locale/loadlocale.c: correct size of _nl_value_type_LC_<category> arrays.
> * locale/programs/ld-time.c: alternative month names support added.
> * locale/programs/locfile-kw.gperf: alt_mon defined.
> * locale/programs/locfile-kw.h: regenerated for alt_mon.
> * locale/programs/locfile-token.h: alt_mon defined.

ChangeLog notation is supposed to be indented exactly one hard tab
before the asterisk or [BZ ...]; only the 'yyyy-mm-dd author <email>'
lines are not indented.  This is important to get right because one of
these years we're going to go to automatic generation of the changelog
from the git logs, and the simpler the script can be, the better.

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

> --- a/locale/langinfo.h
> +++ b/locale/langinfo.h
> @@ -101,6 +101,8 @@ enum
>  #define ABMON_12		ABMON_12
>  
>    /* Long month names.  */
> +  /* Some languages need both standalone and a full-date formatting form,
> +     usually nominative and genitive.  See also ALTMON_x for more details.  */

This comment should be stand-alone and should give sufficient
information for someone skimming the header to know whether MON_n is
what they want.  This means we have to actually decide the question
posed below:

> +  /* 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.  Let's go
ahead and follow POSIX.

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

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

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.

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

----

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

zw