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


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


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