| Message ID | 201904010358.AA04317@tamuki.linet.gr.jp |
|---|---|
| State | New |
| Headers | show |
| Series | Changes around calendar time [BZ #24293][BZ #22964] | expand |
Hello TAMUKI-san, Thank you for working on this patch series. Please see my remarks: 1.04.2019 05:58 TAMUKI Shoichi <tamuki@linet.gr.jp> wrote: > [...] > 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 > [...] > -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; > + > +static const date_t dates[] = > +{ > + { 1, 4, 1988 }, > + { 7, 1, 1989 }, > + { 8, 1, 1989 }, > + { 1, 4, 1990 }, > + { 1, 4, 1997 }, > + { 1, 4, 1998 } > +}; I have learned a new pattern from DJ Delorie. [1] This could be: typedef enum { Jan, Feb, Mar, Apr, May, Jun, Jul, Aug, Sep, Oct, Nov, Dec } Month; typedef struct { const int d; const Month m; const int y; } date_t; static const date_t dates[] = { { 1, Apr, 1988 ), { 7, Jan, 1989 ), { 8, Jan, 1989 ), ... As a result, months would be zero-based and still human-readable. No more need to subtract 1. Also please note Carlos' later remarks about the formatting of DJ's patches which should be applied here as well. [2] > > 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) > +{ > + 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; > +} > + You described this change in your another email: 27.03.2019 04:28 TAMUKI Shoichi <tamuki@linet.gr.jp> wrote: > [...] > 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. I agree with all your other changes but not so much with this one. Somehow I developed a habit that it is a bad design to rely on the global variables because this is an open door for hard to debug side effects. Maybe it is an influence of some object oriented languages like Java which does not have globals deliberately. I would like to revert to the previous design, with the first argument being a date pointer. Which is again inspired by some object oriented designs. Regards, Rafal [1] https://sourceware.org/ml/libc-alpha/2019-03/msg00655.html [2] https://sourceware.org/ml/libc-alpha/2019-03/msg00716.html
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]); >
Carlos, Thank you for your reviews. One question below: 1.04.2019 22:34 Carlos O'Donell <codonell@redhat.com> wrote: > > On 3/31/19 11:58 PM, TAMUKI Shoichi wrote: > [...] > > #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; That's a great idea. What about an idea to define an enum of months like DJ did in one of his recent patches? > > [...] > > @@ -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. That would make months zero-based and convenient to use at the same time. I'm OK with the rest of your comments. Regards, Rafal
On 4/1/19 6:42 PM, Rafal Luzynski wrote: > Carlos, > > Thank you for your reviews. One question below: > > 1.04.2019 22:34 Carlos O'Donell <codonell@redhat.com> wrote: >> >> On 3/31/19 11:58 PM, TAMUKI Shoichi wrote: >> [...] >>> #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; > > That's a great idea. What about an idea to define an enum of > months like DJ did in one of his recent patches? This is just a test case, and I'd like it to land as quickly as possible with the entire patch set. Therefore I don't want to place any restrictions on this moving forward. We can clean this up in another round of edits.
2.04.2019 03:43 Carlos O'Donell <codonell@redhat.com> wrote: > [...] > This is just a test case, and I'd like it to land as quickly > as possible with the entire patch set. Therefore I don't want > to place any restrictions on this moving forward. We can clean > this up in another round of edits. OK, I think you are right. No need for endless polish of the test cases, we have more important things to do. Thank you everybody for working on this! Regards, Rafal
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> #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" +}; 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; + +static const date_t dates[] = +{ + { 1, 4, 1988 }, + { 7, 1, 1989 }, + { 8, 1, 1989 }, + { 1, 4, 1990 }, + { 1, 4, 1997 }, + { 1, 4, 1998 } +}; 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) +{ + 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 + }; + /* Buddhist calendar year to be checked. */ + static const int yrb[] = + { + 2531, 2532, 2532, 2533, 2540, 2541 + }; 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 */ { - 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"; } + else if (i == 1) /* lo_LA */ + era = "\u0e9e.\u0eaa. ", yr = yrb[k], sfx = ""; + else if (i == 2) /* th_TH */ + era = "\u0e1e.\u0e28. ", yr = yrb[k], sfx = ""; 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); + 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); + else + assert (0); /* Unreachable. */ } } @@ -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; strftime (date, sizeof (date), "%F", &ttm); r = strftime (buf, sizeof (buf), formats[j], &ttm); e = strlen (ref[i][j][k]);