From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/12230 Path: news.gmane.org!.POSTED!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] implement strftime GNU extension padding specifiers '_', '-' and '0' Date: Sat, 9 Dec 2017 17:56:50 -0500 Message-ID: <20171209225650.GA1627@brightrain.aerifal.cx> References: <20161122082908.11584-1-timo.teras@iki.fi> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Trace: blaine.gmane.org 1512860224 19217 195.159.176.226 (9 Dec 2017 22:57:04 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sat, 9 Dec 2017 22:57:04 +0000 (UTC) User-Agent: Mutt/1.5.21 (2010-09-15) To: musl@lists.openwall.com Original-X-From: musl-return-12246-gllmg-musl=m.gmane.org@lists.openwall.com Sat Dec 09 23:57:00 2017 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1eNo3I-0004q4-JH for gllmg-musl@m.gmane.org; Sat, 09 Dec 2017 23:57:00 +0100 Original-Received: (qmail 3307 invoked by uid 550); 9 Dec 2017 22:57:03 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 3289 invoked from network); 9 Dec 2017 22:57:02 -0000 Content-Disposition: inline In-Reply-To: <20161122082908.11584-1-timo.teras@iki.fi> Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:12230 Archived-At: 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 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