Re: [PATCH v8 2/2] strftime: Pass the additional flags from "%EY" to "%Ey" [BZ #24096]

[Rafal Luzynski <digitalfreak@lingonborough.com>]

20.01.2019 09:52 TAMUKI Shoichi <tamuki@linet.gr.jp> wrote:
> 
> The full representation of the alternative calendar year (%EY)
> typically includes an internal use of "%Ey".  As a GNU extension,
> apply any flags on "%EY" (e.g. "%_EY", "%-EY") to the internal "%Ey",
> allowing users of "%EY" to control how the year is padded.
> 
> ChangeLog:
> 
> 	[BZ #24096]
> 	* manual/time.texi (strftime): Document "%EC" and "%EY".
> 	* time/Makefile (tests): Add tst-strftime2.
> 	(LOCALES): Add ja_JP.UTF-8, lo_LA.UTF-8, and th_TH.UTF-8.
> 	* time/strftime_l.c (__strftime_internal): Add argument yr_spec to
> 	override padding for "%Ey".
> 	If an optional flag ('_' or '-') is specified to "%EY", interpret the
> 	"%Ey" in the subformat as if decorated with that flag.
> 	* time/tst-strftime2.c: New file.
> ---
>  NEWS                 |   4 ++
>  manual/time.texi     |  11 +++++
>  time/Makefile        |   5 +-
>  time/strftime_l.c    |  19 +++++---
>  time/tst-strftime2.c | 132
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 162 insertions(+), 9 deletions(-)
>  create mode 100644 time/tst-strftime2.c
> 
> diff --git a/NEWS b/NEWS
> index 06e64ddb48..53aee00db8 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -60,6 +60,10 @@ Major new features:
>    alternative year numbers less than 10).  Zero-padding can be
>    overridden with the '_' or '-' flags (which are GNU extensions).
>  
> +* As a GNU extension, the '_' and '-' flags can now be applied to
> +  "%EY" to control how the year number is formatted; they have the
> +  same effect that they would on "%Ey".
> +
>  Deprecated and removed features, and other changes affecting
> compatibility:

I believe this is OK so far.

>  * The glibc.tune tunable namespace has been renamed to glibc.cpu and the
> diff --git a/manual/time.texi b/manual/time.texi
> index 03a8a0e10f..4c93854170 100644
> --- a/manual/time.texi
> +++ b/manual/time.texi
> @@ -1393,6 +1393,10 @@ The preferred calendar time representation for the
> current locale.
>  The century of the year.  This is equivalent to the greatest integer not
>  greater than the year divided by 100.
>  
> +If the @code{E} modifier is specified (@code{%EC}), instead produces
> +the name of the period for the year (e.g. an era name) in the locale's
> +alternative calendar.
> +

"e.g." should be "e.g.@:".  It is important to use "@:" to mark that this
dot is not the end of a sentence and thus control the space width.

>  This format was first standardized by POSIX.2-1992 and by @w{ISO C99}.
>  
>  @item %d
> @@ -1579,6 +1583,13 @@ can be overridden by an explicit field width or by
> the @code{_} and
>  The year as a decimal number, using the Gregorian calendar.  Years
>  before the year @code{1} are numbered @code{0}, @code{-1}, and so on.
>  
> +If the @code{E} modifier is specified (@code{%EY}), instead produces a
> +complete representation of the year according to the locale's
> +alternative calendar.  Generally this will be some combination of the
> +information produced by @code{%EC} and @code{Ey}.  As a GNU extension,
> +the formatting flags @code{_} or @code{-} may be used with this
> +conversion specifier; they affect how the year number is printed.
> +
>  @item %z
>  @w{RFC 822}/@w{ISO 8601:1988} style numeric time zone (e.g.,
>  @code{-0600} or @code{+0100}), or nothing if no time zone is

I believe this is OK but as I've found more issues (see below) I don't
spend much time on the documentation review.

> diff --git a/time/Makefile b/time/Makefile
> index d23ba2dee6..5c6304ece1 100644
> --- a/time/Makefile
> +++ b/time/Makefile
> @@ -43,13 +43,14 @@ tests	:= test_time clocktest tst-posixtz tst-strptime
> tst_wcsftime \
>  	   tst-getdate tst-mktime tst-mktime2 tst-ftime_l tst-strftime \
>  	   tst-mktime3 tst-strptime2 bug-asctime bug-asctime_r bug-mktime1 \
>  	   tst-strptime3 bug-getdate1 tst-strptime-whitespace tst-ftime \
> -	   tst-tzname tst-y2039 bug-mktime4
> +	   tst-tzname tst-y2039 bug-mktime4 tst-strftime2
>  
>  include ../Rules
>  
>  ifeq ($(run-built-tests),yes)
>  LOCALES := de_DE.ISO-8859-1 en_US.ISO-8859-1 ja_JP.EUC-JP fr_FR.UTF-8 \
> -	   es_ES.UTF-8 pl_PL.UTF-8 ru_RU.UTF-8
> +	   es_ES.UTF-8 pl_PL.UTF-8 ru_RU.UTF-8 \
> +	   ja_JP.UTF-8 lo_LA.UTF-8 th_TH.UTF-8
>  include ../gen-locales.mk
>  
>  $(objpfx)tst-ftime_l.out: $(gen-locales)

OK

> diff --git a/time/strftime_l.c b/time/strftime_l.c
> index cbe08e7afb..3d937d3ffd 100644
> --- a/time/strftime_l.c
> +++ b/time/strftime_l.c
> @@ -434,7 +434,7 @@ static CHAR_T const month_name[][10] =
>  #endif
>  
>  static size_t __strftime_internal (CHAR_T *, size_t, const CHAR_T *,
> -				   const struct tm *, bool *
> +				   const struct tm *, int, bool *
>  				   ut_argument_spec
>  				   LOCALE_PARAM) __THROW;
>  

OK

> @@ -456,8 +456,9 @@ my_strftime (CHAR_T *s, size_t maxsize, const CHAR_T
> *format,
>    tmcopy = *tp;
>    tp = &tmcopy;
>  #endif
> +  int yr_spec = 0;		/* Override padding for "%Ey".  */

I think that you don't need this variable now.

>    bool tzset_called = false;
> -  return __strftime_internal (s, maxsize, format, tp, &tzset_called
> +  return __strftime_internal (s, maxsize, format, tp, yr_spec,
> &tzset_called

Consequently, you can pass 0 instead of yr_spec here.

>  			      ut_argument LOCALE_ARG);
>  }
>  #ifdef _LIBC
> @@ -466,7 +467,7 @@ libc_hidden_def (my_strftime)
>  
>  static size_t
>  __strftime_internal (CHAR_T *s, size_t maxsize, const CHAR_T *format,
> -		     const struct tm *tp, bool *tzset_called
> +		     const struct tm *tp, int yr_spec, bool *tzset_called
>  		     ut_argument_spec LOCALE_PARAM)
>  {
>  #if defined _LIBC && defined USE_IN_EXTENDED_LOCALE_MODEL
> @@ -838,11 +839,11 @@ __strftime_internal (CHAR_T *s, size_t maxsize,
> const CHAR_T *format,
>  	  {
>  	    CHAR_T *old_start = p;
>  	    size_t len = __strftime_internal (NULL, (size_t) -1, subfmt,
> -					      tp, tzset_called ut_argument
> -					      LOCALE_ARG);
> +					      tp, yr_spec, tzset_called
> +					      ut_argument LOCALE_ARG);
>  	    add (len, __strftime_internal (p, maxsize - i, subfmt,
> -					   tp, tzset_called ut_argument
> -					   LOCALE_ARG));
> +					   tp, yr_spec, tzset_called
> +					   ut_argument LOCALE_ARG));
>  
>  	    if (to_uppcase)
>  	      while (old_start < p)
> @@ -1273,6 +1274,8 @@ __strftime_internal (CHAR_T *s, size_t maxsize,
> const CHAR_T *format,
>  # else
>  		  subfmt = era->era_format;
>  # endif
> +		  if (pad != 0)
> +		    yr_spec = pad;
>  		  goto subformat;
>  		}
>  #else
> @@ -1294,6 +1297,8 @@ __strftime_internal (CHAR_T *s, size_t maxsize,
> const CHAR_T *format,
>  	      if (era)
>  		{
>  		  int delta = tp->tm_year - era->start_date[0];
> +		  if (yr_spec != 0)
> +		    pad = yr_spec;
>  		  DO_NUMBER (2, (era->offset
>  				 + delta * era->absolute_direction));
>  		}

I believe this is OK.

> diff --git a/time/tst-strftime2.c b/time/tst-strftime2.c
> new file mode 100644
> index 0000000000..57d2144c83
> --- /dev/null
> +++ b/time/tst-strftime2.c
> @@ -0,0 +1,132 @@
> [cut]

I don't quote the rest of the code because I believe it is OK.

Please verify if my suggestions are correct, I am unable to test now.

Regards,

Rafal