* [PATCH 0/3] Staging: speakup: Misc cleanups and code reformatting.
@ 2020-03-23 21:38 Sam Muhammed
2020-03-23 21:38 ` [PATCH 1/3] Staging: speakup: Avoid multiple assignments Sam Muhammed
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Sam Muhammed @ 2020-03-23 21:38 UTC (permalink / raw)
To: William Hubbs, Chris Brannon, Kirk Reiser, Samuel Thibault,
Greg Kroah-Hartman, outreachy-kernel
Cc: Sam Muhammed
Changes to clear multiple Checkpatch.pl CHECKs of
reformatting multiple assignments, proper use of
kmalloc(), and adding identifier names to function
declaration arguments.
All modifications made to match coding style and
to improve readability.
Sam Muhammed (3):
Staging: speakup: Avoid multiple assignments.
Staging: speakup: Use sizeof(*var) in kmalloc().
Staging: speakup: Add identifier name to function declaration
arguments.
drivers/staging/speakup/main.c | 18 ++++++++++++------
drivers/staging/speakup/spk_ttyio.c | 2 +-
drivers/staging/speakup/spk_types.h | 2 +-
3 files changed, 14 insertions(+), 8 deletions(-)
---
2.20.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/3] Staging: speakup: Avoid multiple assignments. 2020-03-23 21:38 [PATCH 0/3] Staging: speakup: Misc cleanups and code reformatting Sam Muhammed @ 2020-03-23 21:38 ` Sam Muhammed 2020-03-23 22:13 ` Samuel Thibault 2020-03-23 21:38 ` [PATCH 2/3] Staging: speakup: Use sizeof(*var) in kmalloc() Sam Muhammed 2020-03-23 21:38 ` [PATCH 3/3] Staging: speakup: Add identifier name to function declaration arguments Sam Muhammed 2 siblings, 1 reply; 10+ messages in thread From: Sam Muhammed @ 2020-03-23 21:38 UTC (permalink / raw) To: William Hubbs, Chris Brannon, Kirk Reiser, Samuel Thibault, Greg Kroah-Hartman, outreachy-kernel Cc: Sam Muhammed Modify the code to avoid multiple assignments by just doing the assignment in two steps. Checkpatch.pl CHECK: multiple assignments should be avoided. Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com> --- drivers/staging/speakup/main.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c index 02471d932d71..6efa0b245bef 100644 --- a/drivers/staging/speakup/main.c +++ b/drivers/staging/speakup/main.c @@ -263,9 +263,12 @@ static unsigned char get_attributes(struct vc_data *vc, u16 *pos) static void speakup_date(struct vc_data *vc) { - spk_x = spk_cx = vc->vc_x; - spk_y = spk_cy = vc->vc_y; - spk_pos = spk_cp = vc->vc_pos; + spk_cx = vc->vc_x; + spk_x = spk_cx; + spk_cy = vc->vc_y; + spk_y = spk_cy; + spk_cp = vc->vc_pos; + spk_pos = spk_cp; spk_old_attr = spk_attr; spk_attr = get_attributes(vc, (u_short *)spk_pos); } @@ -1672,9 +1675,12 @@ static int speak_highlight(struct vc_data *vc) spk_do_flush(); spkup_write(speakup_console[vc_num]->ht.highbuf[hc], speakup_console[vc_num]->ht.highsize[hc]); - spk_pos = spk_cp = speakup_console[vc_num]->ht.rpos[hc]; - spk_x = spk_cx = speakup_console[vc_num]->ht.rx[hc]; - spk_y = spk_cy = speakup_console[vc_num]->ht.ry[hc]; + spk_cp = speakup_console[vc_num]->ht.rpos[hc]; + spk_pos = spk_cp; + spk_cx = speakup_console[vc_num]->ht.rx[hc]; + spk_x = spk_cx; + spk_cy = speakup_console[vc_num]->ht.ry[hc]; + spk_y = spk_cy; return 1; } return 0; --- 2.20.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] Staging: speakup: Avoid multiple assignments. 2020-03-23 21:38 ` [PATCH 1/3] Staging: speakup: Avoid multiple assignments Sam Muhammed @ 2020-03-23 22:13 ` Samuel Thibault 2020-03-23 23:13 ` [Outreachy kernel] " Sam Muhammed 0 siblings, 1 reply; 10+ messages in thread From: Samuel Thibault @ 2020-03-23 22:13 UTC (permalink / raw) To: Sam Muhammed Cc: William Hubbs, Chris Brannon, Kirk Reiser, Greg Kroah-Hartman, outreachy-kernel Hello, Sam Muhammed, le lun. 23 mars 2020 17:38:43 -0400, a ecrit: > Modify the code to avoid multiple assignments by just > doing the assignment in two steps. The original version of the code is considered easier to read. Yes, the tools say multiple assignments should be avoided, but that's not a strict rule :) > Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com> > --- > drivers/staging/speakup/main.c | 18 ++++++++++++------ > 1 file changed, 12 insertions(+), 6 deletions(-) > > diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c > index 02471d932d71..6efa0b245bef 100644 > --- a/drivers/staging/speakup/main.c > +++ b/drivers/staging/speakup/main.c > @@ -263,9 +263,12 @@ static unsigned char get_attributes(struct vc_data *vc, u16 *pos) > > static void speakup_date(struct vc_data *vc) > { > - spk_x = spk_cx = vc->vc_x; > - spk_y = spk_cy = vc->vc_y; > - spk_pos = spk_cp = vc->vc_pos; > + spk_cx = vc->vc_x; > + spk_x = spk_cx; > + spk_cy = vc->vc_y; > + spk_y = spk_cy; > + spk_cp = vc->vc_pos; > + spk_pos = spk_cp; > spk_old_attr = spk_attr; > spk_attr = get_attributes(vc, (u_short *)spk_pos); > } > @@ -1672,9 +1675,12 @@ static int speak_highlight(struct vc_data *vc) > spk_do_flush(); > spkup_write(speakup_console[vc_num]->ht.highbuf[hc], > speakup_console[vc_num]->ht.highsize[hc]); > - spk_pos = spk_cp = speakup_console[vc_num]->ht.rpos[hc]; > - spk_x = spk_cx = speakup_console[vc_num]->ht.rx[hc]; > - spk_y = spk_cy = speakup_console[vc_num]->ht.ry[hc]; > + spk_cp = speakup_console[vc_num]->ht.rpos[hc]; > + spk_pos = spk_cp; > + spk_cx = speakup_console[vc_num]->ht.rx[hc]; > + spk_x = spk_cx; > + spk_cy = speakup_console[vc_num]->ht.ry[hc]; > + spk_y = spk_cy; > return 1; > } > return 0; > --- > 2.20.1 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH 1/3] Staging: speakup: Avoid multiple assignments. 2020-03-23 22:13 ` Samuel Thibault @ 2020-03-23 23:13 ` Sam Muhammed 2020-03-23 23:20 ` Samuel Thibault 0 siblings, 1 reply; 10+ messages in thread From: Sam Muhammed @ 2020-03-23 23:13 UTC (permalink / raw) To: Samuel Thibault Cc: William Hubbs, Chris Brannon, Kirk Reiser, Greg Kroah-Hartman, outreachy-kernel On Mon, 2020-03-23 at 23:13 +0100, Samuel Thibault wrote: > Hello, > > Sam Muhammed, le lun. 23 mars 2020 17:38:43 -0400, a ecrit: > > Modify the code to avoid multiple assignments by just > > doing the assignment in two steps. > > The original version of the code is considered easier to read. > Yes, the tools say multiple assignments should be avoided, but that's > not a strict rule :) Well yes! this got me a bit hesitant, but what got me encouraged to make this change is that i looked for patches for the same change in this driver and i found this commit: f23519843b01fdc266dd70ca29ef0f0a54f8b064 which was applied for the same file main.c, so i thought this driver adopted this style, maintaining a common practice? should i revert this change and drop it from the patchset? Thank you! Sam > > > Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com> > > --- > > drivers/staging/speakup/main.c | 18 ++++++++++++------ > > 1 file changed, 12 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c > > index 02471d932d71..6efa0b245bef 100644 > > --- a/drivers/staging/speakup/main.c > > +++ b/drivers/staging/speakup/main.c > > @@ -263,9 +263,12 @@ static unsigned char get_attributes(struct vc_data *vc, u16 *pos) > > > > static void speakup_date(struct vc_data *vc) > > { > > - spk_x = spk_cx = vc->vc_x; > > - spk_y = spk_cy = vc->vc_y; > > - spk_pos = spk_cp = vc->vc_pos; > > + spk_cx = vc->vc_x; > > + spk_x = spk_cx; > > + spk_cy = vc->vc_y; > > + spk_y = spk_cy; > > + spk_cp = vc->vc_pos; > > + spk_pos = spk_cp; > > spk_old_attr = spk_attr; > > spk_attr = get_attributes(vc, (u_short *)spk_pos); > > } > > @@ -1672,9 +1675,12 @@ static int speak_highlight(struct vc_data *vc) > > spk_do_flush(); > > spkup_write(speakup_console[vc_num]->ht.highbuf[hc], > > speakup_console[vc_num]->ht.highsize[hc]); > > - spk_pos = spk_cp = speakup_console[vc_num]->ht.rpos[hc]; > > - spk_x = spk_cx = speakup_console[vc_num]->ht.rx[hc]; > > - spk_y = spk_cy = speakup_console[vc_num]->ht.ry[hc]; > > + spk_cp = speakup_console[vc_num]->ht.rpos[hc]; > > + spk_pos = spk_cp; > > + spk_cx = speakup_console[vc_num]->ht.rx[hc]; > > + spk_x = spk_cx; > > + spk_cy = speakup_console[vc_num]->ht.ry[hc]; > > + spk_y = spk_cy; > > return 1; > > } > > return 0; > > --- > > 2.20.1 > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH 1/3] Staging: speakup: Avoid multiple assignments. 2020-03-23 23:13 ` [Outreachy kernel] " Sam Muhammed @ 2020-03-23 23:20 ` Samuel Thibault 2020-03-23 23:27 ` Sam Muhammed 0 siblings, 1 reply; 10+ messages in thread From: Samuel Thibault @ 2020-03-23 23:20 UTC (permalink / raw) To: Sam Muhammed Cc: William Hubbs, Chris Brannon, Kirk Reiser, Greg Kroah-Hartman, outreachy-kernel Sam Muhammed, le lun. 23 mars 2020 19:13:16 -0400, a ecrit: > On Mon, 2020-03-23 at 23:13 +0100, Samuel Thibault wrote: > > Hello, > > > > Sam Muhammed, le lun. 23 mars 2020 17:38:43 -0400, a ecrit: > > > Modify the code to avoid multiple assignments by just > > > doing the assignment in two steps. > > > > The original version of the code is considered easier to read. > > Yes, the tools say multiple assignments should be avoided, but that's > > not a strict rule :) > > Well yes! this got me a bit hesitant, but what got me encouraged to > make this change is that i looked for patches for the same change in > this driver and i found this commit: > > f23519843b01fdc266dd70ca29ef0f0a54f8b064 > > which was applied for the same file main.c, so i thought this driver > adopted this style, maintaining a common practice? This is a quite different beast: - value = spk_lastkey = pad_chars[value]; + value = pad_chars[value]; + spk_lastkey = value; there is not much in common between "value" and "spk_lastkey", and much more in common between spk_lastkey and the other spk_* assignments below. > > > - spk_x = spk_cx = vc->vc_x; > > > - spk_y = spk_cy = vc->vc_y; > > > - spk_pos = spk_cp = vc->vc_pos; > > > + spk_cx = vc->vc_x; > > > + spk_x = spk_cx; > > > + spk_cy = vc->vc_y; > > > + spk_y = spk_cy; > > > + spk_cp = vc->vc_pos; > > > + spk_pos = spk_cp; Here, there is a lot in common between spk_x and spk_cx, the current form makes it obvious that we are setting two x variables to vc_x. The proposed form make its much less clear. > should i revert this change and drop it from the patchset? Please drop patch 1 from the 3 yes. Samuel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH 1/3] Staging: speakup: Avoid multiple assignments. 2020-03-23 23:20 ` Samuel Thibault @ 2020-03-23 23:27 ` Sam Muhammed 0 siblings, 0 replies; 10+ messages in thread From: Sam Muhammed @ 2020-03-23 23:27 UTC (permalink / raw) To: Samuel Thibault Cc: William Hubbs, Chris Brannon, Kirk Reiser, Greg Kroah-Hartman, outreachy-kernel On Tue, 2020-03-24 at 00:20 +0100, Samuel Thibault wrote: > Sam Muhammed, le lun. 23 mars 2020 19:13:16 -0400, a ecrit: > > On Mon, 2020-03-23 at 23:13 +0100, Samuel Thibault wrote: > > > Hello, > > > > > > Sam Muhammed, le lun. 23 mars 2020 17:38:43 -0400, a ecrit: > > > > Modify the code to avoid multiple assignments by just > > > > doing the assignment in two steps. > > > > > > The original version of the code is considered easier to read. > > > Yes, the tools say multiple assignments should be avoided, but that's > > > not a strict rule :) > > > > Well yes! this got me a bit hesitant, but what got me encouraged to > > make this change is that i looked for patches for the same change in > > this driver and i found this commit: > > > > f23519843b01fdc266dd70ca29ef0f0a54f8b064 > > > > which was applied for the same file main.c, so i thought this driver > > adopted this style, maintaining a common practice? > > This is a quite different beast: > > - value = spk_lastkey = pad_chars[value]; > + value = pad_chars[value]; > + spk_lastkey = value; > > there is not much in common between "value" and "spk_lastkey", and much > more in common between spk_lastkey and the other spk_* assignments > below. > > > > > > - spk_x = spk_cx = vc->vc_x; > > > > - spk_y = spk_cy = vc->vc_y; > > > > - spk_pos = spk_cp = vc->vc_pos; > > > > + spk_cx = vc->vc_x; > > > > + spk_x = spk_cx; > > > > + spk_cy = vc->vc_y; > > > > + spk_y = spk_cy; > > > > + spk_cp = vc->vc_pos; > > > > + spk_pos = spk_cp; > > Here, there is a lot in common between spk_x and spk_cx, the current > form makes it obvious that we are setting two x variables to vc_x. The > proposed form make its much less clear. Oh, now iam pretty sure about it :) Thank You! Sam > > > should i revert this change and drop it from the patchset? > > Please drop patch 1 from the 3 yes. > > Samuel > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/3] Staging: speakup: Use sizeof(*var) in kmalloc(). 2020-03-23 21:38 [PATCH 0/3] Staging: speakup: Misc cleanups and code reformatting Sam Muhammed 2020-03-23 21:38 ` [PATCH 1/3] Staging: speakup: Avoid multiple assignments Sam Muhammed @ 2020-03-23 21:38 ` Sam Muhammed 2020-03-23 22:14 ` Samuel Thibault 2020-03-23 21:38 ` [PATCH 3/3] Staging: speakup: Add identifier name to function declaration arguments Sam Muhammed 2 siblings, 1 reply; 10+ messages in thread From: Sam Muhammed @ 2020-03-23 21:38 UTC (permalink / raw) To: William Hubbs, Chris Brannon, Kirk Reiser, Samuel Thibault, Greg Kroah-Hartman, outreachy-kernel Cc: Sam Muhammed Modifying struct allocation in kmalloc() to match the coding standards. Checkpatch.pl CHECK: Prefer kmalloc(sizeof(*ldisc_data)...) over kmalloc(sizeof(struct spk_ldisc_data)...) Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com> --- drivers/staging/speakup/spk_ttyio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/speakup/spk_ttyio.c b/drivers/staging/speakup/spk_ttyio.c index 5a9eff08cb96..9b95f77f9265 100644 --- a/drivers/staging/speakup/spk_ttyio.c +++ b/drivers/staging/speakup/spk_ttyio.c @@ -51,7 +51,7 @@ static int spk_ttyio_ldisc_open(struct tty_struct *tty) return -EOPNOTSUPP; speakup_tty = tty; - ldisc_data = kmalloc(sizeof(struct spk_ldisc_data), GFP_KERNEL); + ldisc_data = kmalloc(sizeof(*ldisc_data), GFP_KERNEL); if (!ldisc_data) return -ENOMEM; --- 2.20.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] Staging: speakup: Use sizeof(*var) in kmalloc(). 2020-03-23 21:38 ` [PATCH 2/3] Staging: speakup: Use sizeof(*var) in kmalloc() Sam Muhammed @ 2020-03-23 22:14 ` Samuel Thibault 0 siblings, 0 replies; 10+ messages in thread From: Samuel Thibault @ 2020-03-23 22:14 UTC (permalink / raw) To: Sam Muhammed Cc: William Hubbs, Chris Brannon, Kirk Reiser, Greg Kroah-Hartman, outreachy-kernel Sam Muhammed, le lun. 23 mars 2020 17:38:44 -0400, a ecrit: > Modifying struct allocation in kmalloc() to match the > coding standards. > > Checkpatch.pl CHECK: Prefer kmalloc(sizeof(*ldisc_data)...) > over kmalloc(sizeof(struct spk_ldisc_data)...) > > Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org> Thanks! > --- > drivers/staging/speakup/spk_ttyio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/speakup/spk_ttyio.c b/drivers/staging/speakup/spk_ttyio.c > index 5a9eff08cb96..9b95f77f9265 100644 > --- a/drivers/staging/speakup/spk_ttyio.c > +++ b/drivers/staging/speakup/spk_ttyio.c > @@ -51,7 +51,7 @@ static int spk_ttyio_ldisc_open(struct tty_struct *tty) > return -EOPNOTSUPP; > speakup_tty = tty; > > - ldisc_data = kmalloc(sizeof(struct spk_ldisc_data), GFP_KERNEL); > + ldisc_data = kmalloc(sizeof(*ldisc_data), GFP_KERNEL); > if (!ldisc_data) > return -ENOMEM; > > --- > 2.20.1 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] Staging: speakup: Add identifier name to function declaration arguments. 2020-03-23 21:38 [PATCH 0/3] Staging: speakup: Misc cleanups and code reformatting Sam Muhammed 2020-03-23 21:38 ` [PATCH 1/3] Staging: speakup: Avoid multiple assignments Sam Muhammed 2020-03-23 21:38 ` [PATCH 2/3] Staging: speakup: Use sizeof(*var) in kmalloc() Sam Muhammed @ 2020-03-23 21:38 ` Sam Muhammed 2020-03-23 22:14 ` Samuel Thibault 2 siblings, 1 reply; 10+ messages in thread From: Sam Muhammed @ 2020-03-23 21:38 UTC (permalink / raw) To: William Hubbs, Chris Brannon, Kirk Reiser, Samuel Thibault, Greg Kroah-Hartman, outreachy-kernel Cc: Sam Muhammed The void (*read_buff_add) argument didn't have an identifier name, adding an identifier name to match coding style. Checkpatch.pl CHECK: function definition argument 'u_char' should also have an identifier name. Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com> --- drivers/staging/speakup/spk_types.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/speakup/spk_types.h b/drivers/staging/speakup/spk_types.h index a2fc72c29894..fc6a9416829c 100644 --- a/drivers/staging/speakup/spk_types.h +++ b/drivers/staging/speakup/spk_types.h @@ -189,7 +189,7 @@ struct spk_synth { void (*flush)(struct spk_synth *synth); int (*is_alive)(struct spk_synth *synth); int (*synth_adjust)(struct st_var_header *var); - void (*read_buff_add)(u_char); + void (*read_buff_add)(u_char c); unsigned char (*get_index)(struct spk_synth *synth); struct synth_indexing indexing; int alive; --- 2.20.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] Staging: speakup: Add identifier name to function declaration arguments. 2020-03-23 21:38 ` [PATCH 3/3] Staging: speakup: Add identifier name to function declaration arguments Sam Muhammed @ 2020-03-23 22:14 ` Samuel Thibault 0 siblings, 0 replies; 10+ messages in thread From: Samuel Thibault @ 2020-03-23 22:14 UTC (permalink / raw) To: Sam Muhammed Cc: William Hubbs, Chris Brannon, Kirk Reiser, Greg Kroah-Hartman, outreachy-kernel Sam Muhammed, le lun. 23 mars 2020 17:38:45 -0400, a ecrit: > The void (*read_buff_add) argument didn't have an identifier name, > adding an identifier name to match coding style. > > Checkpatch.pl CHECK: function definition argument 'u_char' > should also have an identifier name. > > Signed-off-by: Sam Muhammed <jane.pnx9@gmail.com> Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > --- > drivers/staging/speakup/spk_types.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/speakup/spk_types.h b/drivers/staging/speakup/spk_types.h > index a2fc72c29894..fc6a9416829c 100644 > --- a/drivers/staging/speakup/spk_types.h > +++ b/drivers/staging/speakup/spk_types.h > @@ -189,7 +189,7 @@ struct spk_synth { > void (*flush)(struct spk_synth *synth); > int (*is_alive)(struct spk_synth *synth); > int (*synth_adjust)(struct st_var_header *var); > - void (*read_buff_add)(u_char); > + void (*read_buff_add)(u_char c); > unsigned char (*get_index)(struct spk_synth *synth); > struct synth_indexing indexing; > int alive; > --- > 2.20.1 > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-03-23 23:27 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-03-23 21:38 [PATCH 0/3] Staging: speakup: Misc cleanups and code reformatting Sam Muhammed 2020-03-23 21:38 ` [PATCH 1/3] Staging: speakup: Avoid multiple assignments Sam Muhammed 2020-03-23 22:13 ` Samuel Thibault 2020-03-23 23:13 ` [Outreachy kernel] " Sam Muhammed 2020-03-23 23:20 ` Samuel Thibault 2020-03-23 23:27 ` Sam Muhammed 2020-03-23 21:38 ` [PATCH 2/3] Staging: speakup: Use sizeof(*var) in kmalloc() Sam Muhammed 2020-03-23 22:14 ` Samuel Thibault 2020-03-23 21:38 ` [PATCH 3/3] Staging: speakup: Add identifier name to function declaration arguments Sam Muhammed 2020-03-23 22:14 ` Samuel Thibault
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.