Re: [PATCH 1/3] printf: support field padding

[Christoffer Dall <christoffer.dall@linaro.org>]

On Fri, Dec 13, 2013 at 11:58:04AM +0100, Andrew Jones wrote:
> Support format flags for field padding, such as "%08x", allowing
> register dumps to be easier on the eyes.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  lib/printf.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 70 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/printf.c b/lib/printf.c
> index 867eb1926f742..dd20f755c01a0 100644
> --- a/lib/printf.c
> +++ b/lib/printf.c
> @@ -6,6 +6,11 @@ typedef struct pstream {
>      int added;
>  } pstream_t;
>  
> +typedef struct strprops {
> +    char pad;
> +    int npad;
> +} strprops_t;
> +
>  static void addchar(pstream_t *p, char c)
>  {
>      if (p->remain) {
> @@ -15,15 +20,37 @@ static void addchar(pstream_t *p, char c)
>      ++p->added;
>  }
>  
> -void print_str(pstream_t *p, const char *s)
> +void print_str(pstream_t *p, const char *s, strprops_t props)
>  {
> +    const char *s_orig = s;
> +    int npad = props.npad;
> +
> +    if (npad > 0) {
> +	npad -= strlen(s_orig);
> +	while (npad > 0) {
> +	    addchar(p, props.pad);
> +	    --npad;
> +	}
> +    }
> +
>      while (*s)
>  	addchar(p, *s++);
> +
> +    if (npad < 0) {
> +	char pad = props.pad;
> +	if (pad == '0') /* ignore '0' flag with '-' flag */
> +	    pad = ' ';

there are only the two options, so you can drop the check if
you like.

> +	npad += strlen(s_orig);
> +	while (npad < 0) {
> +	    addchar(p, pad);
> +	    ++npad;
> +	}
> +    }
>  }
>  
>  static char digits[16] = "0123456789abcdef";
>  
> -void print_int(pstream_t *ps, long long n, int base)
> +void print_int(pstream_t *ps, long long n, int base, strprops_t props)
>  {
>      char buf[sizeof(long) * 3 + 2], *p = buf;
>      int s = 0, i;
> @@ -54,10 +81,11 @@ void print_int(pstream_t *ps, long long n, int base)
>  
>      *p = 0;
>  
> -    print_str(ps, buf);
> +    print_str(ps, buf, props);
>  }
>  
> -void print_unsigned(pstream_t *ps, unsigned long long n, int base)
> +void print_unsigned(pstream_t *ps, unsigned long long n, int base,
> +		    strprops_t props)
>  {
>      char buf[sizeof(long) * 3 + 1], *p = buf;
>      int i;
> @@ -80,7 +108,23 @@ void print_unsigned(pstream_t *ps, unsigned long long n, int base)
>  
>      *p = 0;
>  
> -    print_str(ps, buf);
> +    print_str(ps, buf, props);
> +}
> +
> +static int fmtnum(const char **fmt)
> +{
> +    const char *f = *fmt;
> +    int len = 0, num;
> +
> +    if (*f == '-')
> +	++f, ++len;

oh wow, this deserves a small comment saying that negative values are
used to add trailing padding instead of leading.

> +
> +    while (*f >= '0' && *f <= '9')
> +	++f, ++len;
> +
> +    num = atol(*fmt);
> +    *fmt += len;
> +    return num;
>  }

some funny indentation is back here...  Better check your entire patch
for that.

>  
>  int vsnprintf(char *buf, int size, const char *fmt, va_list va)
> @@ -93,6 +137,9 @@ int vsnprintf(char *buf, int size, const char *fmt, va_list va)
>      while (*fmt) {
>  	char f = *fmt++;
>  	int nlong = 0;
> +	strprops_t props;
> +	memset(&props, 0, sizeof(props));
> +	props.pad = ' ';
>  
>  	if (f != '%') {
>  	    addchar(&s, f);
> @@ -110,41 +157,50 @@ int vsnprintf(char *buf, int size, const char *fmt, va_list va)
>  	case '\0':
>  	    --fmt;
>  	    break;
> +	case '0':
> +	    props.pad = '0';
> +	    ++fmt;
> +	    /* fall through */
> +	case '1'...'9':
> +	case '-':
> +	    --fmt;
> +	    props.npad = fmtnum(&fmt);
> +	    goto morefmt;
>  	case 'l':
>  	    ++nlong;
>  	    goto morefmt;
>  	case 'd':
>  	    switch (nlong) {
>  	    case 0:
> -		print_int(&s, va_arg(va, int), 10);
> +		print_int(&s, va_arg(va, int), 10, props);
>  		break;
>  	    case 1:
> -		print_int(&s, va_arg(va, long), 10);
> +		print_int(&s, va_arg(va, long), 10, props);
>  		break;
>  	    default:
> -		print_int(&s, va_arg(va, long long), 10);
> +		print_int(&s, va_arg(va, long long), 10, props);
>  		break;
>  	    }
>  	    break;
>  	case 'x':
>  	    switch (nlong) {
>  	    case 0:
> -		print_unsigned(&s, va_arg(va, unsigned), 16);
> +		print_unsigned(&s, va_arg(va, unsigned), 16, props);
>  		break;
>  	    case 1:
> -		print_unsigned(&s, va_arg(va, unsigned long), 16);
> +		print_unsigned(&s, va_arg(va, unsigned long), 16, props);
>  		break;
>  	    default:
> -		print_unsigned(&s, va_arg(va, unsigned long long), 16);
> +		print_unsigned(&s, va_arg(va, unsigned long long), 16, props);
>  		break;
>  	    }
>  	    break;
>  	case 'p':
> -	    print_str(&s, "0x");
> -	    print_unsigned(&s, (unsigned long)va_arg(va, void *), 16);
> +	    print_str(&s, "0x", props);
> +	    print_unsigned(&s, (unsigned long)va_arg(va, void *), 16, props);
>  	    break;
>  	case 's':
> -	    print_str(&s, va_arg(va, const char *));
> +	    print_str(&s, va_arg(va, const char *), props);
>  	    break;
>  	default:
>  	    addchar(&s, f);
> -- 
> 1.8.1.4
> 

Besides the formatting stuff:

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>