Messages in this thread | | | Date | Sat, 28 Feb 2009 10:13:05 +0100 | From | Ingo Molnar <> | Subject | Re: [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
| |