This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


29.03.2019 19:37 DJ Delorie <dj@redhat.com> wrote:
> 
> Tweaked the first line to be more accurate (purpose of the test).
> 
> Print mistmatched weekdays as strings if they're valid.
> 
> Fold long test name lines if needed.
> 
> I renamed the buffers used to compare times so that the diagnostics are
> more useful.
> 
> Spaces instead of dashes in time strings.

Thank you for these changes.  Please see my comments below:

> [...]
> diff --git a/time/strptime_l.c b/time/strptime_l.c
> index e19b9a15dd..7436a168b7 100644
> --- a/time/strptime_l.c
> +++ b/time/strptime_l.c

I skip the review of changes in this file because I am unable
to analyze it thoroughly enough.  It looks good at first sight
though and I believe that the test confirms that these changes
are correct.

> diff --git a/time/tst-strftime3.c b/time/tst-strftime3.c
> new file mode 100644
> index 0000000000..274b9c3a65
> --- /dev/null
> +++ b/time/tst-strftime3.c
> @@ -0,0 +1,451 @@
> +/* Data-driven tests for strftime/strptime.

Thank you for this clarification.

> [...]
> +typedef struct Data {
> +  /* A descriptive name of the test. */

Please use two spaces before the closing "*/" consequently.

> [...]
> +   For convenience, mis-matched strings are printed in
> +   paste-compatible format, raw text format, and unicode format.  Use

/s/unicode/Unicode

> +   "" between a hex escape sequence (like \xe8) and a following hex
> +   digit which should be considered as a printable character.
> +
> +   To verify text, save the correct text in a file, and use "od -tx1
> +   -tc file" to see the raw hex values.  */
> +
> +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",
> +    /* <U7D00><U5143><U524D>01<U5E74> 紀元前01年 */
> +    "\xe7\xb4\x80\xe5\x85\x83\xe5\x89\x8d""01\xe5\xb9\xb4" },
> +  { "Japanese era change, BCE/CE, after transition",
> +    1, Jan,  1, Mon, 12, 00, 00, "ja_JP.UTF-8", "%EY",
> +    /* <U897F><U66A6>01<U5E74> 西暦01年 */
> +    "\xe8\xa5\xbf\xe6\x9a\xa6""01\xe5\xb9\xb4" },
> +
> [...]

I don't understand the rule when you decide to use one empty line
and when two empty lines between the elements of the array.  Just
please make sure you use it consequently and deliberately.

> [...]
> +static void
> +print_string_hex (const char *header, const char *str)
> +{
> +  int tictoc = 0;
> +  const char *s = str;
> +  wchar_t w[STRBUFLEN];
> +  size_t i, wlen;
> +
> +  printf("%s : ", header);

Please use a space after "printf".

> +
> +  while (s && *s)
> +    {
> +      /* isgraph() equivalent, but independent of current locale.  */

Same here after "isgraph" although I think I read recently there should
be no "()" after a function name in the documentation.

> +      if (' ' <= *s && *s <= '~')
> +	putchar(*s);
> +      else
> +	{
> +	  if (tictoc)
> +	    printf("\033[36m");
> +	  else
> +	    printf("\033[31m");
> +	  tictoc = ! tictoc;

Same here after "putchar" and "printf".

> +	  printf("\\x%02x\033[0m", (unsigned char)*s);

Same here, additionally please use a space after the type cast
operator "(unsigned char)"

> +	}
> +
> +      ++ s;
> +    }
> +  printf(" - %s\n", str);

Same here.

> +  s = str;
> +  wlen = mbsrtowcs (w, &s, strlen (s), NULL);
> +  printf("%*s", (int)strlen(header) + 3, " ");

Same here:

  printf ("%*s", (int) strlen (header) + 3, " ");

> +  for (i = 0; i < wlen && i < strlen (str); i ++)

OK.

> +    {
> +      if (' ' <= w[i] && w[i] <= '~')
> +	putchar(w[i]);
> +      else
> +	printf("<U%04X>", w[i]);
> +    }
> +  printf("\n");
> +}

Same here - "putchar", "printf".

I hope my suggestions are correct, also I (almost) don't find this
problem in other places in this file.

> [...]
> +const char *weekday_name[] = { "Sun", "Mon", "Tue", "Wed", "Thu", "Fri",
> +			       "Sat" };
> +
> +/* Helper function to create a printable version of struct tm.  */
> +static void
> +tm_to_printed (struct tm *tm, char *buffer)
> +{
> +  const char *wn;
> +  char temp[50];
> +
> +  if (0 <= tm->tm_wday && tm->tm_wday <= 6)
> +    wn = weekday_name[tm->tm_wday];
> +  else
> +    {
> +      wn = temp;
> +      sprintf (temp, "%d", tm->tm_wday);
> +    }

Thank you for this change.  I thought that illegal tm_wday value is
unlikely and if happens it is correct to crash/corrupt/etc.  But
your approach to handle possible illegal value is correct and not
harmful.

> +
> +  snprintf (buffer, TMBUFLEN, "%04d/%02d/%02d %02d:%02d:%02d %s",
> +	    tm->tm_year,
> +	    tm->tm_mon,

Since tm_mon is zero based, I think it should be:

	    tm->tm_mon + 1,

I am sorry for noticing this late.

> [...]
> +      /* Print this just to help debug failures.  */
> +      if (strlen (d->name) + strlen (d->locale) + strlen (d->format)
> +	  + strlen (d->printed) + 4 >= 79)
> +	printf("%s:\n\t%s %s %s\n", d->name, d->locale, d->format, d->printed);
> +      else
> +	printf("%s: %s %s %s\n", d->name, d->locale, d->format, d->printed);

I'm afraid this will not work as expected for two reasons:

* strlen counts bytes rather than characters which is wrong for UTF-8,
* double width characters (quite common in CJK languages) occupy two
  columns on the terminal.

Also, I think we can break the line always, unconditionally, so I suggest
you to break it unconditionally.  Alternatively, feel free not to break
it ever and restore the previous patch.

Also, please use a space after "printf".

> +
> +      tm.tm_year = d->y - 1900;
> +      tm.tm_mon = d->m;
> +      tm.tm_mday = d->d;
> +      tm.tm_wday = d->w;
> +      tm.tm_hour = d->hh;
> +      tm.tm_min = d->mm;
> +      tm.tm_sec = d->ss;
> +      tm.tm_isdst = -1;
> +
> +      /* LC_ALL may interfere with the snprintf in tm_to_printed.  */
> +      if (setlocale (LC_TIME, d->locale) == NULL)
> +	{
> +	  /* See the LOCALES list in the Makefile.  */
> +	  printf ("locale %s does not exist!\n", d->locale);
> +	  exit (EXIT_FAILURE);
> +	}
> +      /* This is just for printing wide characters if there's an error.
>  */
> +      setlocale (LC_CTYPE, d->locale);
> +

OK

> +      rv = strftime (buffer, sizeof(buffer), d->format, &tm);

Please use a space after "sizeof".

> +
> +      TEST_COMPARE (rv, strlen (d->printed));
> +      COMPARE_STRINGS (buffer, d->printed);
> +
> +      /* Copy the original time, so that any fields not affected by
> +	 the call to strptime() will match.  */

See my feedback above about the mention of "isgraph" in the comment.

> +      tm2 = tm;
> +
> +      rvp = strptime (d->printed, d->format, &tm2);
> +
> +      TEST_COMPARE_STRING (rvp, "");
> +
> +      tm_to_printed (&tm, expected_time);
> +      tm_to_printed (&tm2, got_time);
> +      TEST_COMPARE_STRING (got_time, expected_time);
> +    }
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>

OK.

Best regards,

Rafal


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]