This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 2/3] time/tst-strftime2.c: Make the file easier to maintain.
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