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

* 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


\
 
 \ /
  Last update: 2009-02-28 10:19    [W:0.221 / U:0.832 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site