Messages in this thread | | | Date | Sun, 1 Mar 2009 00:11:37 +0100 | From | Frederic Weisbecker <> | Subject | Re: [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.
| |