Re: [PATCH][RFC] vsprintf: unify the format decoding layer for its 3 users

[Ingo Molnar <mingo@elte.hu>]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> > instead? Wouldn't that be nicer? I suspect it would make the 
> > code look nicer too (instead of doing "*base = x", you'd see 
> > "spec->base = x" and it would look less like line noise in 
> > the callee, an the caller could just do a single "struct 
> > format_spec spec = { 0, }" to initialize that thing).
> > 
> > 		Linus
> 
> You're right, that's much proper.
> See the V2 below:

Just a few (very) small code style pet peeves:

> +struct printf_spec {
> +	enum format_type type;
> +	int flags;			/* flags to number() */
> +	int field_width;		/* width of output field */
> +	int base;
> +	/* min. # of digits for integers; max number of chars for from string */
> +	int precision;
> +	int qualifier;
> +};

doesnt it look a bit tidier this way:

 struct printf_spec {
	enum format_type	type;
	int			flags;		/* flags to number()     */
	int			field_width;	/* width of output field */
	int			base;
	int			precision;	/* # of digits/chars     */
	int			qualifier;
 };

?

> +		case '+':
> +			spec->flags |= PLUS;
> +			break;
> +		case ' ':
> +			spec->flags |= SPACE;
> +			break;
> +		case '#':
> +			spec->flags |= SPECIAL;
> +			break;
> +		case '0':
> +			spec->flags |= ZEROPAD;
> +			break;
> +		default:
> +			found = false;

btw., this is one of the cases where i think the original style 
was more useful:

> +		case '+':	spec->flags |= PLUS;  break;
> +		case ' ':	spec->flags |= SPACE; break;
[etc.]

as it's always good to compress repetitive patterns of code.

(If checkpatch complains about this then ignore checkpatch.)

> +	case 'n':
> +		/* FIXME:
> +		* What does C99 say about the overflow case here? */

(this comment looks a bit funny.)

> +		default: {
> +			enum format_type type = spec.type;
> +
> +			if (type == FORMAT_TYPE_LONG_LONG)
> +				num = get_arg(long long);
> +			else if (type == FORMAT_TYPE_ULONG)
> +				num = get_arg(unsigned long);
> +			else if (type == FORMAT_TYPE_LONG)
> +				num = get_arg(unsigned long);
> +			else if (type == FORMAT_TYPE_SIZE_T)
> +				num = get_arg(size_t);
> +			else if (type == FORMAT_TYPE_PTRDIFF)
> +				num = get_arg(ptrdiff_t);
> +			else if (type == FORMAT_TYPE_USHORT)
> +				num = get_arg(unsigned short);
> +			else if (type == FORMAT_TYPE_SHORT)
> +				num = get_arg(short);
> +			else if (type == FORMAT_TYPE_UINT)
> +				num = get_arg(unsigned int);
> +			else
> +				num = get_arg(int);

Wouldnt it be cleaner as a switch() statement and to put into a 
helper function?

Also, could you please resend the current stuff with a 0/ 
description and a diffstat in the 0 mail so that we can all see 
all the patches again and the total impact?

	Ingo