Re: [RFC][PATCH v9 2/6] Implement alternative month names (bug 10871).

[Joseph Myers <joseph at codesourcery dot com>]

On Fri, 27 Oct 2017, Zack Weinberg wrote:

> The code changes look good to me, except for a few small concerns
> listed below.  Also, before landing I would like Joseph Myers (cc:ed)
> to positively confirm that this patch introduces no new ISO C or POSIX
> conformance issues, and will not tie our hands if this or a similar
> feature is standardized in the future.

I believe this patch properly handles namespace issues and avoids any 
problems with improperly changing enum values or making them improperly 
depend on feature test macros defined.  I believe it properly deals with 
allowing existing locales using existing POSIX features to continue to 
work by copying the month values as needed.

There should be a GCC bug filed, if there isn't one already, for allowing 
%OB / %Ob in strftime formats (with appropriate warnings in ISO C pedantic 
modes about being an extension).  Tests may need to disable errors for 
format warnings when built with a GCC version without that support.

> > @@ -1015,6 +1035,10 @@ __strptime_internal (const char *rp, const char *fmt,
> > struct tm *tmp,
> >         case 'O':
> >           switch (*fmt++)
> >             {
> > +           case 'B':
> > +             /* Undo the increment and continue.  */
> > +             fmt--;
> > +             break;
> 
> This is subtly wrong, and I had to read a big chunk of the entire
> function to understand what it was meant to do and how it doesn't
> actually work.  Please change to
> 
> ]         case 'O':
> ]           switch (*fmt++)
> ]             {
> ] +           case 'B':
> ] +             /* Match month name.  Reprocess as plain 'B'.  */
> ] +             fmt--;
> ] +             goto start_over;

Also, the POSIX proposal accepted for issue 8 has strptime %Ob and 
strptime %OB handled the same, despite that POSIX proposal not including 
strftime %Ob.  (This is only an argument for %Ob handling for strptime to 
go in this patch rather than the later one adding alternative abbreviated 
months, however.)

-- 
Joseph S. Myers
joseph@codesourcery.com