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

  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.

	* 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.
---
 time/tst-strftime2.c | 62
+++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 44 insertions(+), 18 deletions(-)

 
@@ -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";
+
 	      if (yrj[k] == 1)
 		sprintf (ref[i][j][k], "%s\xe5\x85\x83\xe5\xb9\xb4", era);
 	      else
@@ -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.  */
+	    }
 	}
 }
 
@@ -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]);
  

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
  

17.03.2019 11:32 TAMUKI Shoichi <tamuki@linet.gr.jp> wrote:
> 
> 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)
> 
> [...]
> > 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>

I don't mind changing this as you suggest but I thought that includes
should be kept alphabetically.  I don't know what is the preferred
convention
in glibc.  Will anybody help here?

> [...]
> > @@ -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.

OK

> > @@ -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.  */
> |  	}
> |  }
> |  

Again I don't mind this but there are different conventions and I am
not sure what is preferred in glibc.

To be honest, I prefer to remove these braces.

Thank you for your feedback, and regards,

Rafal
  

Hello Rafal-san,

From: Rafal Luzynski <digitalfreak@lingonborough.com>
Subject: Re: [PATCH 2/3] time/tst-strftime2.c: Make the file easier to maintain.
Date: Tue, 19 Mar 2019 00:30:57 +0100 (CET)

> > 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>
> 
> I don't mind changing this as you suggest but I thought that includes
> should be kept alphabetically.  I don't know what is the preferred convention
> in glibc.  Will anybody help here?

At first glance, the original includes appear to be in alphabetical
order, but it just happens, and actually the position of time.h is
different.  To improve the readability of source code, they are listed
in loose rules in the order of the procedure.

With relatively large source code, and in sufficiently complex cases,
it is more convenient to be kept in alphabetical order, indeed.

> > 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.  */
> > |  	}
> > |  }
> > |  
> 
> Again I don't mind this but there are different conventions and I am
> not sure what is preferred in glibc.
> 
> To be honest, I prefer to remove these braces.

As a convention in glibc, I think that braces are not necessary for
one-line statements.

Regards,
TAMUKI Shoichi
  

Hello Rafal-san,

I am sorry I can not tell you at once.

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)

> [...]
> +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;
> +}
> +

Since dates[] is a global variable, I think that it will be
simplified by using the index of dates[] instead of the address of
dates[] as an argument for the is_before function.

So maybe something like this:

@@ -62,18 +62,18 @@
 	       [array_length (dates)][100];
 
 static bool
-is_before (const date_t *date, const int d, const int m, const int y)
+is_before (const int i, const int d, const int m, const int y)
 {
-  if (date->y < y)
+  if (dates[i].y < y)
     return true;
-  else if (date->y > y)
+  else if (dates[i].y > y)
     return false;
-  else if (date->m < m)
+  else if (dates[i].m < m)
     return true;
-  else if (date->m > m)
+  else if (dates[i].m > m)
     return false;
   else
-    return date->d < d;
+    return dates[i].d < d;
 }
 
 static void
@@ -103,11 +103,11 @@
 	{
 	  if (i == 0)  /* ja_JP  */
 	    {
-	      if (is_before (&dates[k], 30, 7, 1912))
+	      if (is_before (k, 30, 7, 1912))
 		era = "\xe6\x98\x8e\xe6\xb2\xbb";
-	      else if (is_before (&dates[k], 25, 12, 1926))
+	      else if (is_before (k, 25, 12, 1926))
 		era = "\xe5\xa4\xa7\xe6\xad\xa3";
-	      else if (is_before (&dates[k], 8, 1, 1989))
+	      else if (is_before (k, 8, 1, 1989))
 		era = "\xe6\x98\xad\xe5\x92\x8c";
 	      else
 		era = "\xe5\xb9\xb3\xe6\x88\x90";
@@ -125,7 +125,7 @@
 	    }
 	  else if (i >= 3 && i <= 7)  /* {zh,cmn,hak,nan,lzh}_TW  */
 	    {
-	      if (is_before (&dates[k], 1, 1, 1912))
+	      if (is_before (k, 1, 1, 1912))
 		era = "\xe6\xb0\x91\xe5\x89\x8d";
 	      else
 		era = "\xe6\xb0\x91\xe5\x9c\x8b";

From: TAMUKI Shoichi <tamuki@linet.gr.jp>
Subject: Re: [PATCH 3/3] time: Add tests for Minguo calendar [BZ #24293]
Date: Sun, 24 Mar 2019 20:40:07 +0900

> Question:
> 
> In the typedef struct, which indentation style is appropriate?
> 
> | typedef struct
> | {
> |   const int d, m, y;
> | } date_t;
> | 
> 
> or
> 
> | typedef struct
> |   {
> |     const int d, m, y;
> |   } date_t;
> | 
> 
> ?

According to GNU Coding Standards documentation:

| For struct and enum types, likewise put the braces in column one,
| unless the whole contents fits on one line:
| 
| struct foo
| {
|   int a, b;
| }
| 
| or
| 
| struct foo { int a, b; }

So perhaps maybe something like these:

| static const char *locales[] =
| {
|   "ja_JP.UTF-8", "lo_LA.UTF-8", "th_TH.UTF-8",
|   "zh_TW.UTF-8", "cmn_TW.UTF-8", "hak_TW.UTF-8",
|   "nan_TW.UTF-8", "lzh_TW.UTF-8"
| };

| static const date_t dates[] =
| {
|   {  1,  4, 1910 },
|   { 31, 12, 1911 },
|   { 29,  7, 1912 },
|   { 30,  7, 1912 },
|   {  1,  4, 1913 },
|   {  1,  4, 1988 },
|   {  7,  1, 1989 },
|   {  8,  1, 1989 },
|   {  1,  4, 1990 },
|   {  1,  4, 1997 },
|   {  1,  4, 1998 },
|   {  1,  4, 2010 },
|   {  1,  4, 2011 }
| };

| static void
| mkreftable (void)
| {
|   [...]
|   static const int yrj[] =
|   {
|     43, 44, 45, 1, 2,
|     63, 64, 1, 2, 9, 10, 22, 23
|   };
|   static const int yrb[] =
|   {
|     2453, 2454, 2455, 2455, 2456,
|     2531, 2532, 2532, 2533, 2540, 2541, 2553, 2554
|   };
|   static const int yrc[] =
|   {
|     -2, -1, 1, 1, 2,
|     77, 78, 78, 79, 86, 87, 99, 100
|   };
| 
|   [...]

Regards,
TAMUKI Shoichi