Re: [PATCH 2/3] time/tst-strftime2.c: Make the file easier to maintain.

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

Hello Rafal-san,

From: Rafal Luzynski <digitalfreak@lingonborough.com>
Subject: [PATCH 2/3] time/tst-strftime2.c: Make the file easier to maintain.
Date: Fri, 15 Mar 2019 12:48:34 +0100 (CET)

> Express the years as full Gregorian years (e.g., 1988 instead of 88)
> and months with natural numbers (1-12 rather than 0-11).
> 
> Compare actual dates rather than indexes when selecting the era name.
> 
> Declare the local variable era as a string character pointer rather than
> an array of chars where the actual string is copied which might lead to
> potential buffer overflows in future.

I think these improvements are good.

> 	* time/tst-strftime2.c (date_t): Explicitly define the type.
> 	(dates): Use natural month and year numbers to express a date.
> 	(is_before): New function to compare dates.
> 	(mkreftable): Minor improvements to simplify maintenance.
> 	(do_test): Reflect the changes in dates array.

Looks good to me.

> diff --git a/time/tst-strftime2.c b/time/tst-strftime2.c
> index 3dca2a9..bf5a66d 100644
> --- a/time/tst-strftime2.c
> +++ b/time/tst-strftime2.c
> @@ -19,8 +19,10 @@
>     <http://www.gnu.org/licenses/>.  */
>  
>  #include <array_length.h>
> +#include <assert.h>
>  #include <locale.h>
>  #include <time.h>
> +#include <stdbool.h>
>  #include <stdio.h>
>  #include <string.h>
>  

I care about the order of including header lines.  Because including
stdbool.h and assert.h are used during the reference table creation,
the following order is preferred.

Recommend instead:

| @@ -19,6 +19,8 @@
|     <http://www.gnu.org/licenses/>.  */
|  
|  #include <array_length.h>
| +#include <stdbool.h>
| +#include <assert.h>
|  #include <locale.h>
|  #include <time.h>
|  #include <stdio.h>

> @@ -28,27 +30,44 @@ static const char *locales[] = { "ja_JP.UTF-8", "lo_LA.UTF-8", "th_TH.UTF-8" };
>  
>  static const char *formats[] = { "%EY", "%_EY", "%-EY" };
>  
> -static const struct
> +typedef struct
>  {
>    const int d, m, y;
> -} dates[] =
> +} date_t;
> +
> +static const date_t dates[] =
>    {
> -    { 1, 3, 88 },
> -    { 7, 0, 89 },
> -    { 8, 0, 89 },
> -    { 1, 3, 90 },
> -    { 1, 3, 97 },
> -    { 1, 3, 98 }
> +    { 1, 4, 1988 },
> +    { 7, 1, 1989 },
> +    { 8, 1, 1989 },
> +    { 1, 4, 1990 },
> +    { 1, 4, 1997 },
> +    { 1, 4, 1998 }
>    };

OK.

> +static bool
> +is_before (const date_t *date, const int d, const int m, const int y)
> +{
> +  if (date->y < y)
> +    return true;
> +  else if (date->y > y)
> +    return false;
> +  else if (date->m < m)
> +    return true;
> +  else if (date->m > m)
> +    return false;
> +  else
> +    return date->d < d;
> +}
> +

OK.  Nice idea.

>  static void
>  mkreftable (void)
>  {
>    int i, j, k;
> -  char era[10];
> +  const char *era;
>    static const int yrj[] = { 63, 64, 1, 2, 9, 10 };
>    static const int yrb[] = { 2531, 2532, 2532, 2533, 2540, 2541 };
>  

OK.

> @@ -56,10 +75,13 @@ mkreftable (void)
>      for (j = 0; j < array_length (formats); j++)
>        for (k = 0; k < array_length (dates); k++)
>  	{
> -	  if (i == 0)
> +	  if (i == 0)  /* ja_JP  */
>  	    {
> -	      sprintf (era, "%s", (k < 2) ? "\xe6\x98\xad\xe5\x92\x8c"
> -					  : "\xe5\xb9\xb3\xe6\x88\x90");
> +	      if (is_before (&dates[k], 8, 1, 1989))
> +		era = "\xe6\x98\xad\xe5\x92\x8c";
> +	      else
> +		era = "\xe5\xb9\xb3\xe6\x88\x90";
> +

Please delete the above blank one line.

> @@ -72,16 +94,20 @@ mkreftable (void)
>  		    sprintf (ref[i][j][k], "%s%d\xe5\xb9\xb4", era, yrj[k]);
>  		}
>  	    }
> -	  else if (i == 1)
> +	  else if (i == 1)  /* lo_LA  */
>  	    {
> -	      sprintf (era, "\xe0\xba\x9e\x2e\xe0\xba\xaa\x2e ");
> +	      era = "\xe0\xba\x9e\x2e\xe0\xba\xaa\x2e ";
>  	      sprintf (ref[i][j][k], "%s%d", era, yrb[k]);
>  	    }
> -	  else
> +	  else if (i == 2)  /* th_TH  */
>  	    {
> -	      sprintf (era, "\xe0\xb8\x9e\x2e\xe0\xb8\xa8\x2e ");
> +	      era = "\xe0\xb8\x9e\x2e\xe0\xb8\xa8\x2e ";
>  	      sprintf (ref[i][j][k], "%s%d", era, yrb[k]);
>  	    }
> +	  else
> +	    {
> +	      assert (0);  /* Unreachable.  */
> +	    }
>  	}
>  }
>  

Braces surrounding assert are redundant.

Recommend instead:

| @@ -72,16 +93,18 @@ mkreftable (void)
|  		    sprintf (ref[i][j][k], "%s%d\xe5\xb9\xb4", era, yrj[k]);
|  		}
|  	    }
| -	  else if (i == 1)
| +	  else if (i == 1)  /* lo_LA  */
|  	    {
| -	      sprintf (era, "\xe0\xba\x9e\x2e\xe0\xba\xaa\x2e ");
| +	      era = "\xe0\xba\x9e\x2e\xe0\xba\xaa\x2e ";
|  	      sprintf (ref[i][j][k], "%s%d", era, yrb[k]);
|  	    }
| -	  else
| +	  else if (i == 2)  /* th_TH  */
|  	    {
| -	      sprintf (era, "\xe0\xb8\x9e\x2e\xe0\xb8\xa8\x2e ");
| +	      era = "\xe0\xb8\x9e\x2e\xe0\xb8\xa8\x2e ";
|  	      sprintf (ref[i][j][k], "%s%d", era, yrb[k]);
|  	    }
| +	  else
| +	    assert (0);  /* Unreachable.  */
|  	}
|  }
|  

> @@ -107,8 +133,8 @@ do_test (void)
>  	  for (k = 0; k < array_length (dates); k++)
>  	    {
>  	      ttm.tm_mday = dates[k].d;
> -	      ttm.tm_mon  = dates[k].m;
> -	      ttm.tm_year = dates[k].y;
> +	      ttm.tm_mon  = dates[k].m - 1;
> +	      ttm.tm_year = dates[k].y - 1900;
>  	      strftime (date, sizeof (date), "%F", &ttm);
>  	      r = strftime (buf, sizeof (buf), formats[j], &ttm);
>  	      e = strlen (ref[i][j][k]);

OK.

Thank you for improving the test case.

Regards,
TAMUKI Shoichi