Re: [PATCH] strftime: Allow use of both 'E' and 'O' like "%OEy", "%OEY" [BZ #24453]

[TAMUKI Shoichi <tamuki at linet dot gr dot jp>]

Hello Andreas-san,

Thank you for your review.

From: Andreas Schwab <schwab@suse.de>
Subject: Re: [PATCH] strftime: Allow use of both 'E' and 'O' like "%OEy", "%OEY" [BZ #24453]
Date: Wed, 17 Apr 2019 11:10:04 +0200

> > diff --git a/manual/time.texi b/manual/time.texi
> > index bfa46dd45b..a5ff92dabc 100644
> > --- a/manual/time.texi
> > +++ b/manual/time.texi
> > @@ -1579,6 +1579,13 @@ However, by default it is zero-padded to a minimum of two digits (this
> >  can be overridden by an explicit field width or by the @code{_} and
> >  @code{-} flags).
> >  
> > +As a GNU extension, if both the @code{E} and @code{O} modifiers are
> > +specified (@code{%OEy}), instead produces the year number according to
> > +locale-specific alternative calendar with alternative numeric symbols.
> > +Generally, since the alternative numeric symbols are defined from 0 to
> > +99, when the calendar year number is more than 100, produces the
> > +ordinary number as a fallback.
> 
> The last sentence also applies to %OEY, doesn't it?

Yes, indeed.  I will consider improving the text.

> According to the testcase, the limit is actually locale dependent.

There is the fact that alt_digits array in nl_langinfo function can
contain up to 100 elements.  Generally, alt_digits in localedata are
defined 100 elements.  This is why I described above in time.texi.
On the other hand, only alt_digits in lzh_TW are defined up to 31.  It
seems that they prepared the numbers needed to represent months and
days.  I think it is better to prepare alt_digits in lzh_TW up to 100
elements in the near future so that minutes and seconds can also be
represented.

> This should document both %EOy and %OEy.

As Paul-san suggested, it will be better that just one of the above is
supported.

> > diff --git a/time/strftime_l.c b/time/strftime_l.c
> > index 98c35d58a2..abdcf90789 100644
> > --- a/time/strftime_l.c
> > +++ b/time/strftime_l.c
> > @@ -710,17 +710,11 @@ __strftime_internal (CHAR_T *s, size_t maxsize, const CHAR_T *format,
> >  	}
> >  
> >        /* Check for modifiers.  */
> > -      switch (*f)
> > -	{
> > -	case L_('E'):
> > -	case L_('O'):
> > -	  modifier = *f++;
> > -	  break;
> > -
> > -	default:
> > -	  modifier = 0;
> > -	  break;
> > -	}
> > +      modifier = 0;
> > +      while (*f == L_('E') || *f == L_('O'))
> > +	modifier |= (*f++ == L_('E')) ? L_('E') : L_('O') << 8;
> 
> I think we should support only the spellings %EO and %OE, without
> repetition.

I think so, too.  I will change it to support only the spelling %OE*.

> > +#define MODIFIER_E (modifier & 0xff)
> > +#define MODIFIER_O (modifier >> 8 & 0xff)
> 
> These macros should evaluate to true iff the modifier is present.

Since "modifier" is always present, I think this is no problem.

Regards,
TAMUKI Shoichi