* [PATCH 1/3] staging: panel: (coding style) Matching braces
@ 2014-05-21 12:09 Dominique van den Broeck
2014-05-21 12:10 ` [PATCH 2/3] staging: panel: (coding style) Line alignments and malloc sizeof Dominique van den Broeck
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Dominique van den Broeck @ 2014-05-21 12:09 UTC (permalink / raw)
To: Willy Tarreau, Greg Kroah-Hartman; +Cc: linux-kernel, Dominique van den Broeck
Style-only modifications to comply with checkpatch.pl --strict --file.
. Adds every missing brace in condition statements.
Signed-off-by: Dominique van den Broeck <domdevlin@free.fr>
---
Apply on linux-next tree, above:
commit 4151fa6adc65da14673ece623bbb2acc6936f8be
"Add linux-next specific files for 20140516"
diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index 136671a..1ac5e25 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -711,10 +711,9 @@ static void pin_to_bits(int pin, unsigned char *d_val, unsigned char *c_val)
/* sleeps that many milliseconds with a reschedule */
static void long_sleep(int ms)
{
-
- if (in_interrupt())
+ if (in_interrupt()) {
mdelay(ms);
- else {
+ } else {
current->state = TASK_INTERRUPTIBLE;
schedule_timeout((ms * HZ + 999) / 1000);
}
@@ -1141,13 +1139,13 @@ static inline int handle_lcd_special_code(void)
value = 0;
while (*esc && cgoffset < 8) {
shift ^= 4;
- if (*esc >= '0' && *esc <= '9')
+ if (*esc >= '0' && *esc <= '9') {
value |= (*esc - '0') << shift;
- else if (*esc >= 'A' && *esc <= 'Z')
+ } else if (*esc >= 'A' && *esc <= 'Z') {
value |= (*esc - 'A' + 10) << shift;
- else if (*esc >= 'a' && *esc <= 'z')
+ } else if (*esc >= 'a' && *esc <= 'z') {
value |= (*esc - 'a' + 10) << shift;
- else {
+ } else {
esc++;
continue;
}
@@ -1183,8 +1181,9 @@ static inline int handle_lcd_special_code(void)
esc++;
if (kstrtoul(esc, 10, &lcd_addr_y) < 0)
break;
- } else
+ } else {
break;
+ }
}
lcd_gotoxy();
@@ -1973,10 +1971,11 @@ static int input_name2mask(const char *name, pmask_t *mask, pmask_t *value,
if (isdigit(*name)) {
out = *name - '0';
om |= (1 << out);
- } else if (*name == '-')
+ } else if (*name == '-') {
out = 8;
- else
+ } else {
return 0; /* unknown bit name */
+ }
bit = (out * 5) + in;
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] staging: panel: (coding style) Line alignments and malloc sizeof
2014-05-21 12:09 [PATCH 1/3] staging: panel: (coding style) Matching braces Dominique van den Broeck
@ 2014-05-21 12:10 ` Dominique van den Broeck
2014-05-21 12:10 ` [PATCH 3/3] staging: panel: (coding style) Multiple assignments Dominique van den Broeck
2014-05-26 14:28 ` [PATCH 1/3] staging: panel: (coding style) Matching braces Willy Tarreau
2 siblings, 0 replies; 10+ messages in thread
From: Dominique van den Broeck @ 2014-05-21 12:10 UTC (permalink / raw)
To: Willy Tarreau, Greg Kroah-Hartman; +Cc: linux-kernel, Dominique van den Broeck
Style-only modifications to comply with checkpatch.pl --strict --file.
. Correctly realigns the lines that needed to be ;
. Suppress useless blank rows ;
. Fix sizeof() issues in various -malloc() functions.
Signed-off-by: Dominique van den Broeck <domdevlin@free.fr>
---
Apply on linux-next tree, above:
commit 4151fa6adc65da14673ece623bbb2acc6936f8be
"Add linux-next specific files for 20140516"
diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index 136671a..1ac5e25 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -462,7 +462,7 @@ MODULE_PARM_DESC(lcd_type,
static int lcd_proto = -1;
module_param(lcd_proto, int, 0000);
MODULE_PARM_DESC(lcd_proto,
- "LCD communication: 0=parallel (//), 1=serial, 2=TI LCD Interface");
+ "LCD communication: 0=parallel (//), 1=serial, 2=TI LCD Interface");
static int lcd_charset = -1;
module_param(lcd_charset, int, 0000);
@@ -951,7 +950,6 @@ static void lcd_clear_display(void)
static void lcd_init_display(void)
{
-
lcd_flags = ((lcd_height > 1) ? LCD_FLAG_N : 0)
| LCD_FLAG_D | LCD_FLAG_C | LCD_FLAG_B;
@@ -1313,7 +1312,7 @@ static void lcd_write_char(char c)
}
static ssize_t lcd_write(struct file *file,
- const char __user *buf, size_t count, loff_t *ppos)
+ const char __user *buf, size_t count, loff_t *ppos)
{
const char __user *tmp = buf;
char c;
@@ -1590,7 +1589,6 @@ static void lcd_init(void)
static ssize_t keypad_read(struct file *file,
char __user *buf, size_t count, loff_t *ppos)
{
-
unsigned i = *ppos;
char __user *tmp = buf;
@@ -1615,7 +1613,6 @@ static ssize_t keypad_read(struct file *file,
static int keypad_open(struct inode *inode, struct file *file)
{
-
if (keypad_open_cnt)
return -EBUSY; /* open only once at a time */
@@ -1745,8 +1742,8 @@ static inline int input_state_high(struct logical_input *input)
* release function.
* eg: 0 -(press A)-> A -(press B)-> AB : don't match A's release.
*/
- if (((phys_prev & input->mask) == input->value)
- && ((phys_curr & input->mask) > input->value)) {
+ if (((phys_prev & input->mask) == input->value) &&
+ ((phys_curr & input->mask) > input->value)) {
input->state = INPUT_ST_LOW; /* invalidate */
return 1;
}
@@ -1801,8 +1798,8 @@ static inline void input_state_falling(struct logical_input *input)
{
#if 0
/* FIXME !!! same comment as in input_state_high */
- if (((phys_prev & input->mask) == input->value)
- && ((phys_curr & input->mask) > input->value)) {
+ if (((phys_prev & input->mask) == input->value) &&
+ ((phys_curr & input->mask) > input->value)) {
input->state = INPUT_ST_LOW; /* invalidate */
return;
}
@@ -1960,9 +1957,10 @@ static int input_name2mask(const char *name, pmask_t *mask, pmask_t *value,
while (*name) {
int in, out, bit, neg;
- for (in = 0; (in < sizeof(sigtab)) &&
- (sigtab[in] != *name); in++)
+ for (in = 0; (in < sizeof(sigtab)) && (sigtab[in] != *name);
+ in++)
;
+
if (in >= sizeof(sigtab))
return 0; /* input name not found */
neg = (in & 1); /* odd (lower) names are negated */
@@ -2005,7 +2004,7 @@ static struct logical_input *panel_bind_key(const char *name, const char *press,
{
struct logical_input *key;
- key = kzalloc(sizeof(struct logical_input), GFP_KERNEL);
+ key = kzalloc(sizeof(*key), GFP_KERNEL);
if (!key)
return NULL;
@@ -2043,7 +2042,7 @@ static struct logical_input *panel_bind_callback(char *name,
{
struct logical_input *callback;
- callback = kmalloc(sizeof(struct logical_input), GFP_KERNEL);
+ callback = kmalloc(sizeof(*callback), GFP_KERNEL);
if (!callback)
return NULL;
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] staging: panel: (coding style) Multiple assignments
2014-05-21 12:09 [PATCH 1/3] staging: panel: (coding style) Matching braces Dominique van den Broeck
2014-05-21 12:10 ` [PATCH 2/3] staging: panel: (coding style) Line alignments and malloc sizeof Dominique van den Broeck
@ 2014-05-21 12:10 ` Dominique van den Broeck
2014-05-23 11:33 ` Greg Kroah-Hartman
2014-05-26 14:28 ` [PATCH 1/3] staging: panel: (coding style) Matching braces Willy Tarreau
2 siblings, 1 reply; 10+ messages in thread
From: Dominique van den Broeck @ 2014-05-21 12:10 UTC (permalink / raw)
To: Willy Tarreau, Greg Kroah-Hartman; +Cc: linux-kernel, Dominique van den Broeck
Style-only modifications to comply with checkpatch.pl --strict --file.
. Breaks down compound assignments.
Signed-off-by: Dominique van den Broeck <domdevlin@free.fr>
---
Apply on linux-next tree, above:
commit 4151fa6adc65da14673ece623bbb2acc6936f8be
"Add linux-next specific files for 20140516"
diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index 1ac5e25..8d15b04 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -664,8 +664,8 @@ static void pin_to_bits(int pin, unsigned char *d_val, unsigned char *c_val)
{
int d_bit, c_bit, inv;
- d_val[0] = c_val[0] = d_val[1] = c_val[1] = 0;
- d_val[2] = c_val[2] = 0xFF;
+ d_val[0] = 0 ; c_val[0] = 0 ; d_val[1] = 0 ; c_val[1] = 0;
+ d_val[2] = 0xFF; c_val[2] = 0xFF;
if (pin == 0)
return;
@@ -674,7 +674,7 @@ static void pin_to_bits(int pin, unsigned char *d_val, unsigned char *c_val)
if (inv)
pin = -pin;
- d_bit = c_bit = 0;
+ d_bit = 0; c_bit = 0;
switch (pin) {
case PIN_STROBE: /* strobe, inverted */
@@ -867,7 +867,7 @@ static void lcd_clear_fast_s(void)
{
int pos;
- lcd_addr_x = lcd_addr_y = 0;
+ lcd_addr_x = 0; lcd_addr_y = 0;
lcd_gotoxy();
spin_lock_irq(&pprt_lock);
@@ -879,7 +879,7 @@ static void lcd_clear_fast_s(void)
}
spin_unlock_irq(&pprt_lock);
- lcd_addr_x = lcd_addr_y = 0;
+ lcd_addr_x = 0; lcd_addr_y = 0;
lcd_gotoxy();
}
@@ -888,7 +888,7 @@ static void lcd_clear_fast_p8(void)
{
int pos;
- lcd_addr_x = lcd_addr_y = 0;
+ lcd_addr_x = 0; lcd_addr_y = 0;
lcd_gotoxy();
spin_lock_irq(&pprt_lock);
@@ -915,7 +915,7 @@ static void lcd_clear_fast_p8(void)
}
spin_unlock_irq(&pprt_lock);
- lcd_addr_x = lcd_addr_y = 0;
+ lcd_addr_x = 0; lcd_addr_y = 0;
lcd_gotoxy();
}
@@ -924,7 +924,7 @@ static void lcd_clear_fast_tilcd(void)
{
int pos;
- lcd_addr_x = lcd_addr_y = 0;
+ lcd_addr_x = 0; lcd_addr_y = 0;
lcd_gotoxy();
spin_lock_irq(&pprt_lock);
@@ -936,7 +936,7 @@ static void lcd_clear_fast_tilcd(void)
spin_unlock_irq(&pprt_lock);
- lcd_addr_x = lcd_addr_y = 0;
+ lcd_addr_x = 0; lcd_addr_y = 0;
lcd_gotoxy();
}
@@ -944,7 +944,7 @@ static void lcd_clear_fast_tilcd(void)
static void lcd_clear_display(void)
{
lcd_write_cmd(0x01); /* clear display */
- lcd_addr_x = lcd_addr_y = 0;
+ lcd_addr_x = 0; lcd_addr_y = 0;
/* we must wait a few milliseconds (15) */
long_sleep(15);
}
@@ -1292,7 +1292,7 @@ static void lcd_write_char(char c)
processed = 1;
} else if (!strcmp(lcd_escape, "[H")) {
/* cursor to home */
- lcd_addr_x = lcd_addr_y = 0;
+ lcd_addr_x = 0; lcd_addr_y = 0;
lcd_gotoxy();
processed = 1;
}
@@ -1576,7 +1576,7 @@ static void lcd_init(void)
panel_lcd_print("\x1b[Lc\x1b[Lb\x1b[L*Linux-" UTS_RELEASE "\nPanel-"
PANEL_VERSION);
#endif
- lcd_addr_x = lcd_addr_y = 0;
+ lcd_addr_x = 0; lcd_addr_y = 0;
/* clear the display on the next device opening */
lcd_must_clear = 1;
lcd_gotoxy();
@@ -1953,7 +1953,7 @@ static int input_name2mask(const char *name, pmask_t *mask, pmask_t *value,
char im, om;
pmask_t m, v;
- om = im = m = v = 0ULL;
+ om = 0ULL; im = 0ULL; m = 0ULL; v = 0ULL;
while (*name) {
int in, out, bit, neg;
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] staging: panel: (coding style) Multiple assignments
2014-05-21 12:10 ` [PATCH 3/3] staging: panel: (coding style) Multiple assignments Dominique van den Broeck
@ 2014-05-23 11:33 ` Greg Kroah-Hartman
2014-05-23 23:35 ` [PATCH v2 " Dominique van den Broeck
0 siblings, 1 reply; 10+ messages in thread
From: Greg Kroah-Hartman @ 2014-05-23 11:33 UTC (permalink / raw)
To: Dominique van den Broeck; +Cc: Willy Tarreau, linux-kernel
On Wed, May 21, 2014 at 02:10:01PM +0200, Dominique van den Broeck wrote:
> Style-only modifications to comply with checkpatch.pl --strict --file.
> . Breaks down compound assignments.
>
> Signed-off-by: Dominique van den Broeck <domdevlin@free.fr>
> ---
> Apply on linux-next tree, above:
> commit 4151fa6adc65da14673ece623bbb2acc6936f8be
> "Add linux-next specific files for 20140516"
>
> diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> index 1ac5e25..8d15b04 100644
> --- a/drivers/staging/panel/panel.c
> +++ b/drivers/staging/panel/panel.c
> @@ -664,8 +664,8 @@ static void pin_to_bits(int pin, unsigned char *d_val, unsigned char *c_val)
> {
> int d_bit, c_bit, inv;
>
> - d_val[0] = c_val[0] = d_val[1] = c_val[1] = 0;
> - d_val[2] = c_val[2] = 0xFF;
> + d_val[0] = 0 ; c_val[0] = 0 ; d_val[1] = 0 ; c_val[1] = 0;
> + d_val[2] = 0xFF; c_val[2] = 0xFF;
Honestly, that's harder to read now, so I'll just leave this all alone
and not apply this patch.
Becides, you got the ; wrong...
thanks,
greg k-h
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 3/3] staging: panel: (coding style) Multiple assignments
2014-05-23 11:33 ` Greg Kroah-Hartman
@ 2014-05-23 23:35 ` Dominique van den Broeck
0 siblings, 0 replies; 10+ messages in thread
From: Dominique van den Broeck @ 2014-05-23 23:35 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Willy Tarreau, linux-kernel, Dominique van den Broeck
Style-only modifications to comply with checkpatch.pl --strict --file.
. Breaks down compound assignments.
Signed-off-by: Dominique van den Broeck <domdevlin@free.fr>
---
Resent to lay each variable on its single row.
Other patches have been accepted.
Apply on linux-next tree, above:
commit 4151fa6adc65da14673ece623bbb2acc6936f8be
"Add linux-next specific files for 20140516"
diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
index 1ac5e25..8d15b04 100644
--- a/drivers/staging/panel/panel.c
+++ b/drivers/staging/panel/panel.c
@@ -664,8 +664,12 @@ static void pin_to_bits(int pin, unsigned char *d_val, unsigned char *c_val)
{
int d_bit, c_bit, inv;
- d_val[0] = c_val[0] = d_val[1] = c_val[1] = 0;
- d_val[2] = c_val[2] = 0xFF;
+ d_val[0] = 0;
+ c_val[0] = 0;
+ d_val[1] = 0;
+ c_val[1] = 0;
+ d_val[2] = 0xFF;
+ c_val[2] = 0xFF;
if (pin == 0)
return;
@@ -674,7 +678,8 @@ static void pin_to_bits(int pin, unsigned char *d_val, unsigned char *c_val)
if (inv)
pin = -pin;
- d_bit = c_bit = 0;
+ d_bit = 0;
+ c_bit = 0;
switch (pin) {
case PIN_STROBE: /* strobe, inverted */
@@ -867,7 +872,8 @@ static void lcd_clear_fast_s(void)
{
int pos;
- lcd_addr_x = lcd_addr_y = 0;
+ lcd_addr_x = 0;
+ lcd_addr_y = 0;
lcd_gotoxy();
spin_lock_irq(&pprt_lock);
@@ -879,7 +885,8 @@ static void lcd_clear_fast_s(void)
}
spin_unlock_irq(&pprt_lock);
- lcd_addr_x = lcd_addr_y = 0;
+ lcd_addr_x = 0;
+ lcd_addr_y = 0;
lcd_gotoxy();
}
@@ -888,7 +895,8 @@ static void lcd_clear_fast_p8(void)
{
int pos;
- lcd_addr_x = lcd_addr_y = 0;
+ lcd_addr_x = 0;
+ lcd_addr_y = 0;
lcd_gotoxy();
spin_lock_irq(&pprt_lock);
@@ -915,7 +923,8 @@ static void lcd_clear_fast_p8(void)
}
spin_unlock_irq(&pprt_lock);
- lcd_addr_x = lcd_addr_y = 0;
+ lcd_addr_x = 0;
+ lcd_addr_y = 0;
lcd_gotoxy();
}
@@ -924,7 +933,8 @@ static void lcd_clear_fast_tilcd(void)
{
int pos;
- lcd_addr_x = lcd_addr_y = 0;
+ lcd_addr_x = 0;
+ lcd_addr_y = 0;
lcd_gotoxy();
spin_lock_irq(&pprt_lock);
@@ -936,7 +946,8 @@ static void lcd_clear_fast_tilcd(void)
spin_unlock_irq(&pprt_lock);
- lcd_addr_x = lcd_addr_y = 0;
+ lcd_addr_x = 0;
+ lcd_addr_y = 0;
lcd_gotoxy();
}
@@ -944,7 +955,8 @@ static void lcd_clear_fast_tilcd(void)
static void lcd_clear_display(void)
{
lcd_write_cmd(0x01); /* clear display */
- lcd_addr_x = lcd_addr_y = 0;
+ lcd_addr_x = 0;
+ lcd_addr_y = 0;
/* we must wait a few milliseconds (15) */
long_sleep(15);
}
@@ -1292,7 +1304,8 @@ static void lcd_write_char(char c)
processed = 1;
} else if (!strcmp(lcd_escape, "[H")) {
/* cursor to home */
- lcd_addr_x = lcd_addr_y = 0;
+ lcd_addr_x = 0;
+ lcd_addr_y = 0;
lcd_gotoxy();
processed = 1;
}
@@ -1576,7 +1589,8 @@ static void lcd_init(void)
panel_lcd_print("\x1b[Lc\x1b[Lb\x1b[L*Linux-" UTS_RELEASE "\nPanel-"
PANEL_VERSION);
#endif
- lcd_addr_x = lcd_addr_y = 0;
+ lcd_addr_x = 0;
+ lcd_addr_y = 0;
/* clear the display on the next device opening */
lcd_must_clear = 1;
lcd_gotoxy();
@@ -1953,7 +1967,10 @@ static int input_name2mask(const char *name, pmask_t *mask, pmask_t *value,
char im, om;
pmask_t m, v;
- om = im = m = v = 0ULL;
+ om = 0ULL;
+ im = 0ULL;
+ m = 0ULL;
+ v = 0ULL;
while (*name) {
int in, out, bit, neg;
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] staging: panel: (coding style) Matching braces
2014-05-21 12:09 [PATCH 1/3] staging: panel: (coding style) Matching braces Dominique van den Broeck
2014-05-21 12:10 ` [PATCH 2/3] staging: panel: (coding style) Line alignments and malloc sizeof Dominique van den Broeck
2014-05-21 12:10 ` [PATCH 3/3] staging: panel: (coding style) Multiple assignments Dominique van den Broeck
@ 2014-05-26 14:28 ` Willy Tarreau
2014-05-26 14:45 ` Dominique van den Broeck
2014-05-26 17:28 ` Joe Perches
2 siblings, 2 replies; 10+ messages in thread
From: Willy Tarreau @ 2014-05-26 14:28 UTC (permalink / raw)
To: Dominique van den Broeck; +Cc: Greg Kroah-Hartman, linux-kernel
Hi Dominique,
On Wed, May 21, 2014 at 02:09:59PM +0200, Dominique van den Broeck wrote:
> Style-only modifications to comply with checkpatch.pl --strict --file.
> . Adds every missing brace in condition statements.
>
> Signed-off-by: Dominique van den Broeck <domdevlin@free.fr>
> ---
> Apply on linux-next tree, above:
> commit 4151fa6adc65da14673ece623bbb2acc6936f8be
> "Add linux-next specific files for 20140516"
>
> diff --git a/drivers/staging/panel/panel.c b/drivers/staging/panel/panel.c
> index 136671a..1ac5e25 100644
> --- a/drivers/staging/panel/panel.c
> +++ b/drivers/staging/panel/panel.c
> @@ -711,10 +711,9 @@ static void pin_to_bits(int pin, unsigned char *d_val, unsigned char *c_val)
> /* sleeps that many milliseconds with a reschedule */
> static void long_sleep(int ms)
> {
> -
> - if (in_interrupt())
> + if (in_interrupt()) {
> mdelay(ms);
> - else {
> + } else {
I don't want to be nit-picking, but since we're talking about style...
for me these "} else {" statements are harder to parse than having them
on two lines this way :
}
else {
That's especially more true with "else if" as below :
} else if (*esc >= 'A' && *esc <= 'Z') {
value |= (*esc - 'A' + 10) << shift;
} else if (*esc >= 'a' && *esc <= 'z') {
value |= (*esc - 'a' + 10) << shift;
} else {
I believe that most of the kernel code prefers the two-line format resluting
in this instead :
}
else if (*esc >= 'A' && *esc <= 'Z') {
value |= (*esc - 'A' + 10) << shift;
}
else if (*esc >= 'a' && *esc <= 'z') {
value |= (*esc - 'a' + 10) << shift;
}
else {
It's just a matter of taste I know, but for me they read easier, probably
because the braces do not affect alignment and the lines appear exactly
similar with or without the braces.
Thanks,
Willy
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] staging: panel: (coding style) Matching braces
2014-05-26 14:28 ` [PATCH 1/3] staging: panel: (coding style) Matching braces Willy Tarreau
@ 2014-05-26 14:45 ` Dominique van den Broeck
2014-05-26 15:05 ` Willy Tarreau
2014-05-26 17:28 ` Joe Perches
1 sibling, 1 reply; 10+ messages in thread
From: Dominique van den Broeck @ 2014-05-26 14:45 UTC (permalink / raw)
To: Willy Tarreau; +Cc: Greg Kroah-Hartman, linux-kernel
Hello Willy,
> I don't want to be nit-picking, but since we're talking about style...
> for me these "} else {" statements are harder to parse than having them
> on two lines this way :
> <...>
>
> It's just a matter of taste I know, but for me they read easier, probably
> because the braces do not affect alignment and the lines appear exactly
> similar with or without the braces.
I don't mind at all about this.
Even if I'm into C code for quite a long time now, I'm still new in kernel
development (just completed the Eudyptula Challenge) and I thought it could
be both a harmless and useful way to start contributing and get used with it
to focus a bit on ./checkpatch.pl suggestions (which is the actual entity to
blame about it).
This is the reason why I submitted the patch but it's not a personal
preference. If you prefer these braces laid out the older way, I'll let
them as is next time. If there's another usages I should know about, just
let me know.
Cheers.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] staging: panel: (coding style) Matching braces
2014-05-26 14:45 ` Dominique van den Broeck
@ 2014-05-26 15:05 ` Willy Tarreau
0 siblings, 0 replies; 10+ messages in thread
From: Willy Tarreau @ 2014-05-26 15:05 UTC (permalink / raw)
To: Dominique van den Broeck; +Cc: Greg Kroah-Hartman, linux-kernel
On Mon, May 26, 2014 at 04:45:05PM +0200, Dominique van den Broeck wrote:
>
> Hello Willy,
>
> > I don't want to be nit-picking, but since we're talking about style...
> > for me these "} else {" statements are harder to parse than having them
> > on two lines this way :
> > <...>
> >
> > It's just a matter of taste I know, but for me they read easier, probably
> > because the braces do not affect alignment and the lines appear exactly
> > similar with or without the braces.
>
> I don't mind at all about this.
>
> Even if I'm into C code for quite a long time now, I'm still new in kernel
> development (just completed the Eudyptula Challenge) and I thought it could
> be both a harmless and useful way to start contributing and get used with it
> to focus a bit on ./checkpatch.pl suggestions (which is the actual entity to
> blame about it).
>
> This is the reason why I submitted the patch but it's not a personal
> preference. If you prefer these braces laid out the older way, I'll let
> them as is next time. If there's another usages I should know about, just
> let me know.
You shouldn't bother too much about these style issues in fact. You'll
ask 4 people, you'll get 4 different opinions and will end up with
frustration depending on who will reply with just a "NAK", and sometimes
your change will cause a regression because it's easy to to a mistake
when just changing style.
The best thing to do in general if you want to slowly get into the kernel
is to focus on features you're actually using and which do not work the
optimal way. It's then easy to start by adding debugging code all over
the drive, find where the functions are called from and progressively
get an idea of what does what. Buying cheap small devices is a fun way
to do this BTW :-)
Willy
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] staging: panel: (coding style) Matching braces
2014-05-26 14:28 ` [PATCH 1/3] staging: panel: (coding style) Matching braces Willy Tarreau
2014-05-26 14:45 ` Dominique van den Broeck
@ 2014-05-26 17:28 ` Joe Perches
2014-05-26 17:49 ` Willy Tarreau
1 sibling, 1 reply; 10+ messages in thread
From: Joe Perches @ 2014-05-26 17:28 UTC (permalink / raw)
To: Willy Tarreau; +Cc: Dominique van den Broeck, Greg Kroah-Hartman, linux-kernel
On Mon, 2014-05-26 at 16:28 +0200, Willy Tarreau wrote:
> I believe that most of the kernel code prefers the two-line format resluting
> in this instead :
>
> }
> else if (*esc >= 'A' && *esc <= 'Z') {
> value |= (*esc - 'A' + 10) << shift;
> }
> else if (*esc >= 'a' && *esc <= 'z') {
> value |= (*esc - 'a' + 10) << shift;
> }
> else {
grep shows what kernel style is most used.
Your form:
}
else if (...) {
(This shows 3 lines per instance)
$ grep-2.5.4 -rP --include=*.[ch] '}\n[ \t]*else if.*{\n' * | wc -l
909
Generally used form:
} else if (...) {
(mostly shows 2 lines per instance)
$ grep-2.5.4 -rP --include=*.[ch] '} else if.*{\n' * | wc -l
31653
That's ~50:1 preference for "} else if (...) {"
> It's just a matter of taste I know,
true enough.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] staging: panel: (coding style) Matching braces
2014-05-26 17:28 ` Joe Perches
@ 2014-05-26 17:49 ` Willy Tarreau
0 siblings, 0 replies; 10+ messages in thread
From: Willy Tarreau @ 2014-05-26 17:49 UTC (permalink / raw)
To: Joe Perches; +Cc: Dominique van den Broeck, Greg Kroah-Hartman, linux-kernel
On Mon, May 26, 2014 at 10:28:10AM -0700, Joe Perches wrote:
> On Mon, 2014-05-26 at 16:28 +0200, Willy Tarreau wrote:
> > I believe that most of the kernel code prefers the two-line format resluting
> > in this instead :
> >
> > }
> > else if (*esc >= 'A' && *esc <= 'Z') {
> > value |= (*esc - 'A' + 10) << shift;
> > }
> > else if (*esc >= 'a' && *esc <= 'z') {
> > value |= (*esc - 'a' + 10) << shift;
> > }
> > else {
>
> grep shows what kernel style is most used.
>
> Your form:
> }
> else if (...) {
>
> (This shows 3 lines per instance)
> $ grep-2.5.4 -rP --include=*.[ch] '}\n[ \t]*else if.*{\n' * | wc -l
> 909
>
> Generally used form:
> } else if (...) {
>
> (mostly shows 2 lines per instance)
> $ grep-2.5.4 -rP --include=*.[ch] '} else if.*{\n' * | wc -l
> 31653
>
> That's ~50:1 preference for "} else if (...) {"
Ah indeed, I wouldn't have thought that. Thanks for correcting me, Joe!
Willy
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-05-26 17:49 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-21 12:09 [PATCH 1/3] staging: panel: (coding style) Matching braces Dominique van den Broeck
2014-05-21 12:10 ` [PATCH 2/3] staging: panel: (coding style) Line alignments and malloc sizeof Dominique van den Broeck
2014-05-21 12:10 ` [PATCH 3/3] staging: panel: (coding style) Multiple assignments Dominique van den Broeck
2014-05-23 11:33 ` Greg Kroah-Hartman
2014-05-23 23:35 ` [PATCH v2 " Dominique van den Broeck
2014-05-26 14:28 ` [PATCH 1/3] staging: panel: (coding style) Matching braces Willy Tarreau
2014-05-26 14:45 ` Dominique van den Broeck
2014-05-26 15:05 ` Willy Tarreau
2014-05-26 17:28 ` Joe Perches
2014-05-26 17:49 ` Willy Tarreau
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox