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: [PATCH 2/4] time/tst-strftime2.c: Make the file easier to maintain


On 3/31/19 11:58 PM, TAMUKI Shoichi wrote:

OK for master with the following changes:
- Use Co-authored-by in git commit message.
- Add Rafal's name as a secondary author in Changelog.
- Add and use test_locale enum.
- Change assert to FAIL_EXIT1.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

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.

The original author: Rafal Luzynski <digitalfreak@lingonborough.com>
^^^^

Please use:
Co-authored-by: Rafal Luzynski <digitalfreak@lingonborough.com>

In the final commit message.



ChangeLog:

Please add "Rafal Luzynski <digitalfreak@lingonborough.com>" to the
ChangeLog.

	* 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 | 107 ++++++++++++++++++++++++++++++++-------------------
  1 file changed, 67 insertions(+), 40 deletions(-)

diff --git a/time/tst-strftime2.c b/time/tst-strftime2.c
index 3dca2a997f..f537d93ba4 100644
--- a/time/tst-strftime2.c
+++ b/time/tst-strftime2.c
@@ -19,69 +19,96 @@
     <http://www.gnu.org/licenses/>.  */
#include <array_length.h>
+#include <stdbool.h>
+#include <assert.h>
+#include <stdlib.h>

OK.

  #include <locale.h>
  #include <time.h>
  #include <stdio.h>
  #include <string.h>
-static const char *locales[] = { "ja_JP.UTF-8", "lo_LA.UTF-8", "th_TH.UTF-8" };
+static const char *locales[] =
+{
+  "ja_JP.UTF-8", "lo_LA.UTF-8", "th_TH.UTF-8"
+};

Add enum and use below, this prevents constant and comment
getting out of sync.

/* Must match locale index into locales array.  */
typedef enum
  {
    ja_JP, lo_LA, th_TH
  } test_locale;

static const char *formats[] = { "%EY", "%_EY", "%-EY" }; -static const struct
+typedef struct
  {
    const int d, m, y;
-} dates[] =
-  {
-    { 1, 3, 88 },
-    { 7, 0, 89 },
-    { 8, 0, 89 },
-    { 1, 3, 90 },
-    { 1, 3, 97 },
-    { 1, 3, 98 }
-  };
+} date_t;

OK.

+
+static const date_t dates[] =
+{
+  {  1,  4, 1988 },
+  {  7,  1, 1989 },
+  {  8,  1, 1989 },
+  {  1,  4, 1990 },
+  {  1,  4, 1997 },
+  {  1,  4, 1998 }
+};

OK. Full year.

static char ref[array_length (locales)][array_length (formats)]
  	       [array_length (dates)][100];
+static bool
+is_before (const int i, const int d, const int m, const int y)

OK. Access of a global is fine here, this is just a test.

+{
+  if (dates[i].y < y)
+    return true;
+  else if (dates[i].y > y)
+    return false;
+  else if (dates[i].m < m)
+    return true;
+  else if (dates[i].m > m)
+    return false;
+  else
+    return dates[i].d < d;
+}
+
  static void
  mkreftable (void)
  {
-  int i, j, k;
-  char era[10];
-  static const int yrj[] = { 63, 64, 1, 2, 9, 10 };
-  static const int yrb[] = { 2531, 2532, 2532, 2533, 2540, 2541 };
+  int i, j, k, yr;
+  const char *era, *sfx;
+  /* Japanese era year to be checked.  */
+  static const int yrj[] =
+  {
+    63, 64, 1, 2, 9, 10
+  };

OK.

+  /* Buddhist calendar year to be checked.  */
+  static const int yrb[] =
+  {
+    2531, 2532, 2532, 2533, 2540, 2541
+  };

OK.

for (i = 0; i < array_length (locales); i++)
      for (j = 0; j < array_length (formats); j++)
        for (k = 0; k < array_length (dates); k++)
  	{
-	  if (i == 0)
-	    {
-	      sprintf (era, "%s", (k < 2) ? "\xe6\x98\xad\xe5\x92\x8c"
-					  : "\xe5\xb9\xb3\xe6\x88\x90");
-	      if (yrj[k] == 1)
-		sprintf (ref[i][j][k], "%s\xe5\x85\x83\xe5\xb9\xb4", era);
-	      else
-		{
-		  if (j == 0)
-		    sprintf (ref[i][j][k], "%s%02d\xe5\xb9\xb4", era, yrj[k]);
-		  else if (j == 1)
-		    sprintf (ref[i][j][k], "%s%2d\xe5\xb9\xb4", era, yrj[k]);
-		  else
-		    sprintf (ref[i][j][k], "%s%d\xe5\xb9\xb4", era, yrj[k]);
-		}
-	    }
-	  else if (i == 1)
+	  if (i == 0)  /* ja_JP  */

Use if (i == ja_JP)

Remove comment.

  	    {
-	      sprintf (era, "\xe0\xba\x9e\x2e\xe0\xba\xaa\x2e ");
-	      sprintf (ref[i][j][k], "%s%d", era, yrb[k]);
+	      era = (is_before (k, 8, 1, 1989)) ? "\u662d\u548c"
+						: "\u5e73\u6210";
+	      yr = yrj[k], sfx = "\u5e74";

OK. For the record I'm *OK* with the use of \uXXXX in the test sources,
and if we have a bug we'll take it up with libcpp. I won't proscribe
the use of encoded byte strings, they have their uses and isolate us
from common mode failures in testing.

  	    }
+	  else if (i == 1)  /* lo_LA  */
+	    era = "\u0e9e.\u0eaa. ", yr = yrb[k], sfx = "";

Use if (i == lo_LA)

Remove comment.

+	  else if (i == 2)  /* th_TH  */
+	    era = "\u0e1e.\u0e28. ", yr = yrb[k], sfx = "";

Use if (i == th_TH)

Remove comment.

  	  else
-	    {
-	      sprintf (era, "\xe0\xb8\x9e\x2e\xe0\xb8\xa8\x2e ");
-	      sprintf (ref[i][j][k], "%s%d", era, yrb[k]);
-	    }
+	    assert (0);  /* Unreachable.  */
+	  if (yr == 1)
+	    sprintf (ref[i][j][k], "%s\u5143%s", era, sfx);

OK. First year in ja_JP.

+	  else if (j == 0)
+	    sprintf (ref[i][j][k], "%s%02d%s", era, abs (yr), sfx);
+	  else if (j == 1)
+	    sprintf (ref[i][j][k], "%s%2d%s", era, abs (yr), sfx);
+	  else if (j == 2)
+	    sprintf (ref[i][j][k], "%s%d%s", era, abs (yr), sfx);

OK. These are the formats being computed.

+	  else
+	    assert (0);  /* Unreachable.  */

Remove assert and use:

FAIL_EXIT1 ("Invalid table index!");

  	}
  }
@@ -107,8 +134,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;

OK. I like the use of proper years and 1-index months for test data.

  	      strftime (date, sizeof (date), "%F", &ttm);
  	      r = strftime (buf, sizeof (buf), formats[j], &ttm);
  	      e = strlen (ref[i][j][k]);



--
Cheers,
Carlos.


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