Messages in this thread | | | Date | Sun, 1 Nov 2009 23:45:49 +0100 | From | Frederic Weisbecker <> | Subject | Re: [PATCH] vsprintf: reduce code size, clean up |
| |
On Sun, Nov 01, 2009 at 03:01:40PM -0200, André Goddard Rosa wrote: > +static char null[] = "(null)"; > +
This should be static const. Also, may be chose a better name, as "null" is too much generic and somehow collide with NULL.
null_str ?
> @@ -735,8 +737,9 @@ static char *ip6_compressed_string(char *p, const > char *addr) > p = pack_hex_byte(p, hi); > else > *p++ = hex_asc_lo(hi); > + p = pack_hex_byte(p, lo); > } > - if (hi || lo > 0x0f) > + else if (lo > 0x0f) > p = pack_hex_byte(p, lo); > else > *p++ = hex_asc_lo(lo);
I'm not sure the above is really a simplification. It's more a matter of personal preference :-)
But the previous version factorized the action.
> @@ -822,30 +825,34 @@ static char *pointer(const char *fmt, char *buf, > char *end, void *ptr, > struct printf_spec spec) > { > if (!ptr) > - return string(buf, end, "(null)", spec); > + return string(buf, end, null, spec); > > - switch (*fmt) { > - case 'F': > + switch (TOLOWER(*fmt)) { > case 'f': > + /* or case 'F' */ > ptr = dereference_function_descriptor(ptr); > - case 's': > /* Fallthrough */ > - case 'S': > + case 's': > + /* or case 'S' */ > return symbol_string(buf, end, ptr, spec, *fmt); > case 'R': > return resource_string(buf, end, ptr, spec);
What happens if we have %pr ? It will behave like %pR but it shouldn't.
I don't think this is a good thing to do this switch(TO_LOWER(..)) thing.
We might want to change the behaviour for x but not for X in the future (x being whatever letter in %px) and this code factorization breaks such flexibility.
That also means we'll need to handle exceptions like %pr and perhaps we'll even need to revert these changes once we add another %px without a matching %pX
> @@ -970,8 +977,8 @@ precision: > qualifier: > /* get the conversion qualifier */ > spec->qualifier = -1; > - if (*fmt == 'h' || *fmt == 'l' || *fmt == 'L' || > - *fmt == 'Z' || *fmt == 'z' || *fmt == 't') { > + if (*fmt == 'h' || TOLOWER(*fmt) == 'l' || > + TOLOWER(*fmt) == 'z' || *fmt == 't') { > spec->qualifier = *fmt++; > if (unlikely(spec->qualifier == *fmt)) { > if (spec->qualifier == 'l') { > @@ -1038,7 +1045,7 @@ qualifier: > spec->type = FORMAT_TYPE_LONG; > else > spec->type = FORMAT_TYPE_ULONG; > - } else if (spec->qualifier == 'Z' || spec->qualifier == 'z') { > + } else if (TOLOWER(spec->qualifier) == 'z') {
But there the TO_LOWER is fine.
> @@ -1798,13 +1802,14 @@ int vsscanf(const char * buf, const char * > fmt, va_list args) > } > } > } > - base = 10; > - is_sign = 0; > > if (!*fmt || !*str) > break; > > - switch(*fmt++) { > + base = 10; > + is_sign = 0; > + > + switch (TOLOWER(*fmt++)) { > case 'c': > { > char *s = (char *) va_arg(args,char*);
Please don't do that, this breaks the scanf format.
What happens if we have %C or %S or...?
Ok, the rest looks good. But you should split up this patch into several more targeted patches, because:
1) Several more divided/targeted/focused patches are easier to review, and people will be more keen to review them.
2) vsprintf.c is a bit sensible as a tiny change might break printk() and other things, which means you need the desired effect in 1) :)
3) It will make the bisection easier, which makes 2) smoother to deal with (if you break printk)
I would suggest you:
- Factorize null string - Whitespaces/checkpatch.pl fixes - TO_LOWER things - Move local vars to bloc local vars - CASE statement factorization ?
Hm?
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |