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

[Frederic Weisbecker <fweisbec@gmail.com>]

On Sat, Feb 28, 2009 at 10:13:05AM +0100, Ingo Molnar wrote:
> 
> * 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

Ok, for all these comments. Except I'm not sure that it is needed to export this part
in a helper, it means 3 new different helpers with two of them having a pointer to a va_list
in their parameters.
A pointer to va_list is legal but doesn't seem to me much proper.

Unless you have some objections, I will repost the new addressed version and if you still
think these parts should be exported to helpers, then I will do it.