Message ID | 1025917329.248288.1552650514094@poczta.nazwa.pl |
---|---|
State | Superseded |
Headers |
Received: (qmail 89334 invoked by alias); 15 Mar 2019 11:48:49 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 88271 invoked by uid 89); 15 Mar 2019 11:48:43 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-16.5 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=011, 7513, HX-Languages-Length:3612, month X-HELO: shared-ano163.rev.nazwa.pl X-Spam-Score: -1 Date: Fri, 15 Mar 2019 12:48:34 +0100 (CET) From: Rafal Luzynski <digitalfreak@lingonborough.com> To: libc-alpha@sourceware.org Cc: TAMUKI Shoichi <tamuki@linet.gr.jp>, Felix Yan <felixonmars@archlinux.org> Message-ID: <1025917329.248288.1552650514094@poczta.nazwa.pl> In-Reply-To: <988338284.248045.1552650382964@poczta.nazwa.pl> References: <988338284.248045.1552650382964@poczta.nazwa.pl> Subject: [PATCH 2/3] time/tst-strftime2.c: Make the file easier to maintain. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit |
Commit Message
Rafal Luzynski
March 15, 2019, 11:48 a.m. UTC
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]);
Comments
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
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> @@ -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 } }; static char ref[array_length (locales)][array_length (formats)] [array_length (dates)][100]; +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; +} + 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 };