[PATCH 2/4] time/tst-strftime2.c: Make the file easier to maintain
Carlos O'Donell
codonell@redhat.com
Mon Apr 1 20:34:00 GMT 2019
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.
More information about the Libc-alpha
mailing list