staging:panel: Fixed coding conventions.
in [Kernel]
|
Prev: Announce loop-AES-v3.4a file/swap crypto package
Next: staging/wlags49_hs: Fix build error when CONFIG_SYSFS is not set
From: Willy Tarreau on 11 Jun 2010 08:50 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(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Willy Tarreau on 12 Jun 2010 01:00
On Sat, Jun 12, 2010 at 12:27:36AM +0300, Henri H�kkinen wrote: > Fixed coding convention issues as reported by checkpatch.pl tool > on the file `panel.c'. Moved LCD special code handling from the > function `lcd_write' into function `handle_lcd_special_code'. Also > moved the handling of INPUT_ST_HIGH and INPUT_ST_FALLING states from > the function `panel_process_input' into functions `input_state_high' > and `input_state_falling'. > > Signed-off-by: Henri H�kkinen <henuxd(a)gmail.com> Acked-by: Willy Tarreau <w(a)1wt.eu> Thank you Henri, this version is *a lot* better than previous one, and the result is overall cleaner than the original. Regards, Willy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |