Re: [v4] Fix strptime era handling, add more era tests [BZ #24394]

[Rafal Luzynski <digitalfreak at lingonborough dot com>]

Hello DJ,

Thank you for your contribution.  My review is incomplete but
as far as I can see Carlos has already done a great job here.
This is only an additional feedback. Please see below.

29.03.2019 04:35 DJ Delorie <dj@redhat.com> wrote:
> [...]
> Added the BCE/CE transition.  I won't guarantee that the day of the week
> is correct for that one; I used what strftime returned.  I'm not testing
> the day of week code here ;-)

Do we need to initialize any weekday number at all?  It's difficult
to figure out the correct weekday number for past dates without using
a calendar.

> [...]
> diff --git a/time/Makefile b/time/Makefile
> index 5c6304ece1..2ca206309d 100644
> --- a/time/Makefile
> +++ b/time/Makefile
> @@ -43,7 +43,7 @@ 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-strftime2
> +	   tst-tzname tst-y2039 bug-mktime4 tst-strftime2 tst-strftime3
>  
>  include ../Rules
>  

Correct, this Makefile already generates all necessary locales.
No need to add more.

> [...]
> diff --git a/time/tst-strftime3.c b/time/tst-strftime3.c
> new file mode 100644
> index 0000000000..ac5ddf7b80
> --- /dev/null
> +++ b/time/tst-strftime3.c
> @@ -0,0 +1,433 @@
> +/* Test for strftime, esp Japenese era name changes.

Is "esp" a shortcut for "especially"?  Unless I am the only one confused
here, would you mind using the full version?

> [...]
> +/* These exist for the convenience of writing the test data, because
> +   zero-based vs one-based.  */
> +typedef enum {
> +  Sun, Mon, Tue, Wed, Thu, Fri, Sat
> +} WeekDay;
> +
> +typedef enum {
> +  Jan, Feb, Mar, Apr, May, Jun, Jul, Aug, Sep, Oct, Nov, Dec
> +} Month;

This pattern is excellent.  I hope you don't mind that I use for
the other tests for strftime() which I'm currently working on.

> [...]
> +const Data data[] = {
> +
> +  { "Baseline test",
> +    2019, Mar, 27, Wed, 14,  3, 22, "en_US.ISO-8859-1", "%Y-%m-%d %T",
> +    "2019-03-27 14:03:22" },
> +
> +
> +  { "Japanese era change, BCE/CE, before transition",
> +    0, Dec,  31, Sun, 12, 00, 00, "ja_JP.UTF-8", "%EY",

Do we need two ^ spaces here and three spaces in the dates where
a day number is one-digit?  If not, can you please remove one space?

> +/* Helper function to create a printable version of struct tm.  */
> +static void
> +tm_to_printed (struct tm *tm, char *buffer)
> +{
> +  snprintf (buffer, TMBUFLEN, "%04d/%02d/%02d-%02d:%02d:%02d-%d",
> +	    tm->tm_year,
> +	    tm->tm_mon,
> +	    tm->tm_mday,
> +	    tm->tm_hour,
> +	    tm->tm_min,
> +	    tm->tm_sec,
> +	    tm->tm_wday);
> +}

Would it be helpful to print the weekday name (even abbreviated)
instead of a weekday number which may be confusing? Suggestion:

const char *weekday_name[] = { "Sun", "Mon", "Tue", "Wed", "Thu", "Fri",
"Sat" };

[...]

  snprintf (buffer, TMBUFLEN, "%04d/%02d/%02d-%02d:%02d:%02d, %s",
	    tm->tm_year,
	    tm->tm_mon,
	    tm->tm_mday,
	    tm->tm_hour,
	    tm->tm_min,
	    tm->tm_sec,
	    weekday_name[tm->tm_wday]);

What about a space rather than a dash between a day number and an hour?

> +static int
> +do_test (void)
> +{
> [...]
> +      /* Print this just to help debug failures.  */
> +      printf("%s: %s %s %s\n", d->name, d->locale, d->format,
> d->printed);

This often produces the lines which exceed 80 columns.  Can we
have a linebreak in the middle?  Suggestions:

      printf("%s:\n%s %s %s\n", d->name, d->locale, d->format, d->printed);

or

      printf("%s: %s\n%s %s\n", d->name, d->locale, d->format, d->printed);

or any other variant.  Additional tabs or spaces are welcome as well.

Will it be easy to add more locales and more calendars to this test file?
I think it is possible but I'm also thinking on whether it can be easier.
No suggestion from me at the moment, feel free not to change anything
if you don't have any suggestion either.

Regards,

Rafal