Re: [PATCH] implement strftime GNU extension padding specifiers '_', '-' and '0'

[Rich Felker <dalias@libc.org>]

On Tue, Nov 22, 2016 at 10:29:08AM +0200, Timo Teräs wrote:
> ---
> For review. GNU and most BSDs seem to implement these extensions.
> 
> This applies them to numbers only. There might be few cases where this
> could be applied more, but I think covers most of the uses. Is there any
> test suite I could verify this against?

I'm reasonably conviced this doesn't break anything -- for now I just
did a test of all the standard formats before and after, with explicit
padding and width specifiers for the year ones specified to support
it, and didn't see any change in output. I'm not sure all of them are
right now, but they're no worse with the patch, so that's ok.

I do have some questions about inefficiency:

>  src/time/strftime.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/src/time/strftime.c b/src/time/strftime.c
> index e103e02..14fe6f5 100644
> --- a/src/time/strftime.c
> +++ b/src/time/strftime.c
> @@ -48,12 +48,12 @@ static int week_num(const struct tm *tm)
>  const char *__tm_to_tzname(const struct tm *);
>  size_t __strftime_l(char *restrict, size_t, const char *restrict, const struct tm *restrict, locale_t);
>  
> -const char *__strftime_fmt_1(char (*s)[100], size_t *l, int f, const struct tm *tm, locale_t loc)
> +const char *__strftime_fmt_1(char (*s)[100], size_t *l, int f, const struct tm *tm, locale_t loc, int pad)
>  {
>  	nl_item item;
>  	long long val;
>  	const char *fmt = "-";
> -	int width = 2;
> +	int width = 2, def_pad = '0';
>  
>  	switch (f) {
>  	case 'a':
> @@ -79,15 +79,14 @@ const char *__strftime_fmt_1(char (*s)[100], size_t *l, int f, const struct tm *
>  	case 'C':
>  		val = (1900LL+tm->tm_year) / 100;
>  		goto number;
> +	case 'e':
> +		def_pad = '_';
>  	case 'd':
>  		val = tm->tm_mday;
>  		goto number;
>  	case 'D':
>  		fmt = "%m/%d/%y";
>  		goto recu_strftime;
> -	case 'e':
> -		*l = snprintf(*s, sizeof *s, "%2d", tm->tm_mday);
> -		return *s;

This looks like a nice change.

>  	case 'F':
>  		fmt = "%Y-%m-%d";
>  		goto recu_strftime;
> @@ -200,7 +199,12 @@ const char *__strftime_fmt_1(char (*s)[100], size_t *l, int f, const struct tm *
>  		return 0;
>  	}
>  number:
> -	*l = snprintf(*s, sizeof *s, "%0*lld", width, val);
> +	switch (pad ? pad : def_pad) {
> +	case '-': *l = snprintf(*s, sizeof *s, "%lld", val); break;
> +	case '_': *l = snprintf(*s, sizeof *s, "%*lld", width, val); break;
> +	case '0':
> +	default:  *l = snprintf(*s, sizeof *s, "%0*lld", width, val); break;
> +	}

This looks like gratuitous duplication of the call point; instead, the
format string can just vary, and there are only two possibilities:
%*lld and %0*lld. The '-' case can just be implemented by setting
width to 0.

>  	return *s;
>  nl_strcat:
>  	fmt = __nl_langinfo_l(item, loc);
> @@ -221,7 +225,7 @@ size_t __strftime_l(char *restrict s, size_t n, const char *restrict f, const st
>  	char buf[100];
>  	char *p;
>  	const char *t;
> -	int plus;
> +	int pad, plus;
>  	unsigned long width;
>  	for (l=0; l<n; f++) {
>  		if (!*f) {
> @@ -233,6 +237,8 @@ size_t __strftime_l(char *restrict s, size_t n, const char *restrict f, const st
>  			continue;
>  		}
>  		f++;
> +		pad = 0;
> +		if (*f == '-' || *f == '_' || *f == '0') pad = *f++;
>  		if ((plus = (*f == '+'))) f++;

I think this is OK since POSIX leaves it unspecified what happens with
more than one flag character.

>  		width = strtoul(f, &p, 10);
>  		if (*p == 'C' || *p == 'F' || *p == 'G' || *p == 'Y') {
> @@ -242,7 +248,7 @@ size_t __strftime_l(char *restrict s, size_t n, const char *restrict f, const st
>  		}
>  		f = p;
>  		if (*f == 'E' || *f == 'O') f++;
> -		t = __strftime_fmt_1(&buf, &k, *f, tm, loc);
> +		t = __strftime_fmt_1(&buf, &k, *f, tm, loc, pad);
>  		if (!t) break;
>  		if (width) {
>  			for (; *t=='+' || *t=='-' || (*t=='0'&&t[1]); t++, k--);
> -- 
> 2.10.2

If you're tired of working and waiting on this and just want me to
commit it as-is and make any improvements later, just let me know.
I'll try to adapt the (freeform, non-checking) tests I did into
something that can go into libc-test.

Rich