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/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


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