Re: [PATCH] staging:panel: Fixed coding conventions.

[Willy Tarreau]

On Fri, Jun 11, 2010 at 03:21:49PM +0300, Henri Häkkinen wrote:
> Fixed many coding convention issues as reported by checkpatch.pl tool.
> Many of the cleanups were hacky due to deeply nasted control structures.

NAK. With such a "cleanup", it becomes even harder to read, understand
and debug. Enforcing the 80-chars limit on an existing code generally
involves restructuring it differently. This might be appropriate, but
pushing all the right chars on the 80th column just for the sake of not 
crossing the bound is the worst that can be done.

Also, single-line comments such as below area really boring to read
when converted to multi-line :

>  #define KEYPAD_BUFFER	64
>  #define INPUT_POLL_TIME	(HZ/50)	/* poll the keyboard this every second */
>  #define KEYPAD_REP_START	(10)	/* a key starts to repeat after this times INPUT_POLL_TIME */
>  #define KEYPAD_REP_DELAY	(2)	/* a key repeats this times INPUT_POLL_TIME */

becomes :

> +#define INPUT_POLL_TIME		(HZ/50)	/* poll the keyboard this every
> +					   second */
> +#define KEYPAD_REP_START	(10)	/* a key starts to repeat after
> +					   this times INPUT_POLL_TIME */
> +#define KEYPAD_REP_DELAY	(2)	/* a key repeats this times
> +					   INPUT_POLL_TIME */

Better shorten the comments or put them on their own line.

Also, I particularly hate changes as below. Looks, it's counter-productive,
you added 9 lines of code, which means you reduced a 80x25 terminal by half.
And the tests and statements are unreadable :

> @@ -1182,12 +1252,21 @@ static ssize_t lcd_write(struct file *file,
>  						value = 0;
>  						while (*esc && cgoffset < 8) {
>  							shift ^= 4;
> -							if (*esc >= '0' && *esc <= '9')
> -								value |= (*esc - '0') << shift;
> -							else if (*esc >= 'A' && *esc <= 'Z')
> -								value |= (*esc - 'A' + 10) << shift;
> -							else if (*esc >= 'a' && *esc <= 'z')
> -								value |= (*esc - 'a' + 10) << shift;
> +							if (*esc >= '0' &&
> +							    *esc <= '9')
> +								value |=
> +								  (*esc - '0')
> +								  << shift;
> +							else if (*esc >= 'A' &&
> +								 *esc <= 'Z')
> +								value |=
> +								  (*esc - 'A' +
> +								   10) << shift;
> +							else if (*esc >= 'a' &&
> +								 *esc <= 'z')
> +								value |=
> +								  (*esc - 'a' +
> +								   10) << shift;

If the line length really cause you trouble, see if you can move that
to an inline function and keep the code readable.

Thanks,
Willy

--
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/