Re: [PATCH] nstrftime: merge glibc strftime changes

[TAMUKI Shoichi]

Hello Paul-san,

Thank you for your porting Glibc strftime changes to the
implementation in Gnulib.

Let me just make a few comments to make it better code.

From: Paul Eggert <address@hidden>
Subject: [PATCH] nstrftime: merge glibc strftime changes
Date: Thu, 21 Feb 2019 20:08:46 -0800

> [...]
> diff --git a/lib/nstrftime.c b/lib/nstrftime.c
> index fd731e1c4..a45d809b2 100644
> --- a/lib/nstrftime.c
> +++ b/lib/nstrftime.c
> @@ -418,7 +418,7 @@ iso_week_days (int yday, int wday)
>  
>  static size_t __strftime_internal (STREAM_OR_CHAR_T *, STRFTIME_ARG (size_t)
>                                     const CHAR_T *, const struct tm *,
> -                                   bool, bool *
> +                                   int, bool, bool *

For the implementation of nstrftime.c in Gnulib, upcase is added as an
argument of __strftime_internal function compared to strftime_l.c in
Glibc.

This (upcase) argument is used mainly for the process described in the
beginning of format_char, such as %a, %A, %[bh], %B, etc.

On the other hand, tzset_called argument is used in the process
described at the end of format_char, i.e. %z.

Therefore, I think these arguments are in this order.

Since yr_spec argument to be added this time is used in %EY and %Ey of
format_char, it is better to make the argument order the same.

Recommend instead:

| @@ -418,7 +418,7 @@ iso_week_days (int yday, int wday)
|  
|  static size_t __strftime_internal (STREAM_OR_CHAR_T *, STRFTIME_ARG (size_t)
|                                     const CHAR_T *, const struct tm *,
| -                                   bool, bool *
| +                                   bool, int, bool *
|                                     extra_args_spec LOCALE_PARAM);
|  
|  /* Write information from TP into S according to the format

> [...]
> @@ -433,7 +433,7 @@ my_strftime (STREAM_OR_CHAR_T *s, STRFTIME_ARG (size_t 
> maxsize)
>               const struct tm *tp extra_args_spec LOCALE_PARAM)
>  {
>    bool tzset_called = false;
> -  return __strftime_internal (s, STRFTIME_ARG (maxsize) format, tp,
> +  return __strftime_internal (s, STRFTIME_ARG (maxsize) format, tp, 0,

Ditto.  Recommend instead:

| @@ -434,7 +434,7 @@ my_strftime (STREAM_OR_CHAR_T *s, STRFTIME_ARG (size_t 
maxsize)
|  {
|    bool tzset_called = false;
|    return __strftime_internal (s, STRFTIME_ARG (maxsize) format, tp,
| -                              false, &tzset_called extra_args LOCALE_ARG);
| +                              false, 0, &tzset_called extra_args LOCALE_ARG);
|  }
|  #if defined _LIBC && ! FPRINTFTIME
|  libc_hidden_def (my_strftime)

> [...]
> @@ -446,7 +446,8 @@ libc_hidden_def (my_strftime)
>  static size_t
>  __strftime_internal (STREAM_OR_CHAR_T *s, STRFTIME_ARG (size_t maxsize)
>                       const CHAR_T *format,
> -                     const struct tm *tp, bool upcase, bool *tzset_called
> +                     const struct tm *tp, int yr_spec,
> +                     bool upcase, bool *tzset_called

Ditto.  Recommend instead:

| @@ -446,8 +446,8 @@ libc_hidden_def (my_strftime)
|  static size_t
|  __strftime_internal (STREAM_OR_CHAR_T *s, STRFTIME_ARG (size_t maxsize)
|                       const CHAR_T *format,
| -                     const struct tm *tp, bool upcase, bool *tzset_called
| -                     extra_args_spec LOCALE_PARAM)
| +                     const struct tm *tp, bool upcase, int yr_spec,
| +                     bool *tzset_called extra_args_spec LOCALE_PARAM)
|  {
|  #if defined _LIBC && defined USE_IN_EXTENDED_LOCALE_MODEL
|    struct __locale_data *const current = loc->__locales[LC_TIME];

> [...]
> @@ -863,13 +864,13 @@ __strftime_internal (STREAM_OR_CHAR_T *s, STRFTIME_ARG 
> (size_t maxsize)
>          subformat:
>            {
>              size_t len = __strftime_internal (NULL, STRFTIME_ARG ((size_t) 
> -1)
> -                                              subfmt,
> -                                              tp, to_uppcase, tzset_called
> +                                              subfmt, tp, yr_spec,
> +                                              to_uppcase, tzset_called
>                                                extra_args LOCALE_ARG);
>              add (len, __strftime_internal (p,
>                                             STRFTIME_ARG (maxsize - i)
> -                                           subfmt,
> -                                           tp, to_uppcase, tzset_called
> +                                           subfmt, tp, yr_spec,
> +                                           to_uppcase, tzset_called

Ditto.  Recommend instead:

| @@ -864,13 +864,15 @@ __strftime_internal (STREAM_OR_CHAR_T *s, STRFTIME_ARG 
(size_t maxsize)
|            {
|              size_t len = __strftime_internal (NULL, STRFTIME_ARG ((size_t) 
-1)
|                                                subfmt,
| -                                              tp, to_uppcase, tzset_called
| -                                              extra_args LOCALE_ARG);
| +                                              tp, to_uppcase, yr_spec,
| +                                              tzset_called extra_args
| +                                              LOCALE_ARG);
|              add (len, __strftime_internal (p,
|                                             STRFTIME_ARG (maxsize - i)
|                                             subfmt,
| -                                           tp, to_uppcase, tzset_called
| -                                           extra_args LOCALE_ARG));
| +                                           tp, to_uppcase, yr_spec,
| +                                           tzset_called extra_args
| +                                           LOCALE_ARG));
|            }
|            break;
|  

Thank you for your consideration.

Regards,
TAMUKI Shoichi