lkml.org 
[lkml]   [2009]   [Feb]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH][RFC] vsprintf: unify the format decoding layer for its 3 users
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.



\
 
 \ /
  Last update: 2009-03-01 00:15    [W:0.108 / U:1.468 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site