Re: kernel lib parser: cleaned up code and fixed redundancies

[Jesper Juhl]

On Tue, 28 Dec 2004, Jonathan Ho wrote:

> Just cleaned up code and fixed variable assignment redundancies.
> 
> Signed-off-by: <jonathanho15@xxxxxxxxx>
> 
> ----------------------------------
> 
> --- linux-2.6.10/lib/parser.c.orig    Fri Dec 24 13:34:33 2004
> +++ linux-2.6.10/lib/parser.c    Tue Dec 28 12:35:38 2004
> @@ -104,8 +104,7 @@ int match_token(char *s, match_table_t t
> {
>     struct match_token *p;
> 
> -    for (p = table; !match_one(s, p->pattern, args) ; p++)
> -        ;
> +    for (p = table; !match_one(s, p->pattern, args); p++);
> 
Personally I prefer the ";" on a line by itself - make it more 
obvious that the for loop is supposed to be empty and thus more 
readable. If you move the semicolon it it looks more like an error on 
casual inspection.
Personally I'd probably have changed the existing code to something like 
this if at all : 

for (p = table; !match_one(s, p->pattern, args); p++)
	; /* empty body */

Then it's really obvious.


> static int match_number(substring_t *s, int *result, int base)
> {
> -    char *endp;
> -    char *buf;
> -    int ret;
> +    char *endp, *buf;
> +    int ret = 0;
> 
>     buf = kmalloc(s->to - s->from + 1, GFP_KERNEL);
>     if (!buf)
> @@ -132,7 +130,6 @@ static int match_number(substring_t *s,
>     memcpy(buf, s->from, s->to - s->from);
>     buf[s->to - s->from] = '\0';
>     *result = simple_strtol(buf, &endp, base);
> -    ret = 0;
>     if (endp == buf)
>         ret = -EINVAL;
>     kfree(buf);
> 
Why move the assignment of ret? the way it was the assignment would 
only be done when nessesary (we can bail out of the function before the 
poing where ret was set to 0) - with your code we'll always set ret to 0 
even when we bail out early with return -ENOMEM; and ret is not used at 
all.


-- 
Jesper Juhl 


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/