Re: [Outreachy kernel] [PATCH v2] staging: speakup: Comparison to NULL could be written
On Thu, Mar 02, 2017 at 08:37:21AM -0800, Alison Schofield wrote:
> On Thu, Mar 02, 2017 at 02:13:02PM +0530, Arushi Singhal wrote:
> > Fixed coding style for null comparisons in speakup driver to be more
> > consistant with the rest of the kernel coding style.
> >
> > Signed-off-by: Arushi Singhal
> > ---
> > changes in v2
> > - fixed coding style error and upto the coding style.
>
> Hi Arushi,
>
> Take another look at Joe's message about checkpatch on the patch
> file. Looks like you missed that.
Arushi,
Looking further, seems you only missed the spelling warning:
WARNING: 'consistant' may be misspelled - perhaps 'consistent'?
You would be missing this if you are running checkpatch on your
.c file, but not doing the checkpatch "hook" from the firstpatch
tutorial. That hook would catch this. And, you can just catch
it by running checkpatch on the patch file after it's built.
(The hook is a painless failsafe...recommend using it.)
alisons
>
> Here's some tips on styling your commit and log messages:
>
> You've probably seen feedback of putting the message into
> the "imperative". ie...it's as if you are directing/commanding
> what is to happen. Best way I've found to get that to sink in
> is to look at your predecessors...and look at more than one
> because poor messages do sneak in occasionally.
>
> For this one, I might do this:
>
> > staging/speakup$ gitpretty *.c | grep NULL
> > (my alias: alias gitpretty='git log --pretty=oneline --abbrev-commit')
> >
> > d3da1cb staging: speakup: (coding style) Rewrite comparisons to NULL
> > a90624c Staging: speakup: kobjects.c: Remove explicit NULL comparison
> > 114885e Staging: speakup: serialio.c: Remove explicit NULL comparison
> > 562c4798 Staging: speakup: devsynth.c: Remove explicit NULL comparison
> > ff52fc3 Staging: speakup: varhandlers.c: Remove explicit NULL comparison
> >
> Then, go further into one that looks like it might match your change:
>
> > staging/speakup$ git log a90624c
> > commit a90624cf253cc74e9464b42d54aa4825575edefe
> > Author: Shraddha Barke
> > Date: Fri Sep 11 11:32:28 2015 +0530
> >
> > Staging: speakup: kobjects.c: Remove explicit NULL comparison
> > >
> > Remove explicit NULL comparison and write it in its simpler form.
> >
> >
>
> This would give me confidence in the commit message and log.
> And, since I tend toward the obsessive ;), if the next day I do this
> fix in another directory, I would repeat this process on that directory
> and file because styles can vary by driver/subsystem. Perhaps not on
> such a simple fix, but more so as you dive deeper.
>
> alisons
>
> >
> > drivers/staging/speakup/fakekey.c | 2 +-
> > drivers/staging/speakup/kobjects.c | 2 +-
> > drivers/staging/speakup/main.c | 38
> > +++---
> > 3 files changed, 21 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/staging/speakup/fakekey.c
> > b/drivers/staging/speakup/fakekey.c
> > index d76da0a1382c..294c74b47224 100644
> > --- a/drivers/staging/speakup/fakekey.c
> > +++ b/drivers/staging/speakup/fakekey.c
> > @@ -56,7 +56,7 @@ int speakup_add_virtual_keyboard(void)
> >
> > void speakup_remove_virtual_keyboard(void)
> > {
> > - if (virt_keyboard != NULL) {
> > + if (virt_keyboard) {
> > input_unregister_device(virt_keyboard);
> > virt_keyboard = NULL;
> > }
> > diff --git a/drivers/staging/speakup/kobjects.c
> > b/drivers/staging/speakup/kobjects.c
> > index 5d871ec3693c..2fef55569bfd 100644
> > --- a/drivers/staging/speakup/kobjects.c
> > +++ b/drivers/staging/speakup/kobjects.c
> > @@ -391,7 +391,7 @@ static ssize_t synth_store(struct kobject *kobj, struct
> > kobj_attribute *attr,
> > len--;
> > new_synth_name[len] = '\0';
> > spk_strlwr(new_synth_name);
> > - if ((synth != NULL) && (!strcmp(new_synth_name, synth->name))) {
> > + if (synth && !strcmp(new_synth_name, synth->name)) {
> > pr_warn("%s already in use\n", new_synth_name);
> > } else if (synth_init(new_synth_name) != 0) {
> > pr_warn("failed to init synth %s\n", new_synth_name);
> > diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
> > index c2f70ef5b9b3..a12ec2b061fe 100644
> > --- a/drivers/staging/speakup/main.c
> > +++ b/drivers/staging/speakup/main.c
> > @@ -299,7 +299,7 @@ static void speakup_shut_up(struct vc_data *vc)
> > spk_shut_up |= 0x01;
> > spk_parked &= 0xfe;
> > speakup_date(vc);
> > - if (synth != NULL)
> > + if (synth)
> > spk_do_flush();
> > }
> >
> > @@ -441,7 +441,7 @@ static void speak_char(u_char ch)
> > synth_printf("%s", spk_str_caps_stop);
> > return;
> > }
> > - if (cp == NULL) {
> > + if (!cp) {
> > pr_info("speak_char: cp == NULL!\n");
> > return;
> > }
> > @@ -1157,7 +1157,7 @@ static void do_handle_shift(struct vc_data *vc,
> > u_char value, char up_flag)
Re: [Outreachy kernel] [PATCH v2] staging: speakup: Comparison to NULL could be written
On Thu, Mar 02, 2017 at 02:13:02PM +0530, Arushi Singhal wrote:
> Fixed coding style for null comparisons in speakup driver to be more
> consistant with the rest of the kernel coding style.
>
> Signed-off-by: Arushi Singhal
> ---
> changes in v2
> - fixed coding style error and upto the coding style.
Hi Arushi,
Take another look at Joe's message about checkpatch on the patch
file. Looks like you missed that.
Here's some tips on styling your commit and log messages:
You've probably seen feedback of putting the message into
the "imperative". ie...it's as if you are directing/commanding
what is to happen. Best way I've found to get that to sink in
is to look at your predecessors...and look at more than one
because poor messages do sneak in occasionally.
For this one, I might do this:
> staging/speakup$ gitpretty *.c | grep NULL
> (my alias: alias gitpretty='git log --pretty=oneline --abbrev-commit')
>
> d3da1cb staging: speakup: (coding style) Rewrite comparisons to NULL
> a90624c Staging: speakup: kobjects.c: Remove explicit NULL comparison
> 114885e Staging: speakup: serialio.c: Remove explicit NULL comparison
> 562c4798 Staging: speakup: devsynth.c: Remove explicit NULL comparison
> ff52fc3 Staging: speakup: varhandlers.c: Remove explicit NULL comparison
>
Then, go further into one that looks like it might match your change:
> staging/speakup$ git log a90624c
> commit a90624cf253cc74e9464b42d54aa4825575edefe
> Author: Shraddha Barke
> Date: Fri Sep 11 11:32:28 2015 +0530
>
> Staging: speakup: kobjects.c: Remove explicit NULL comparison
> >
> Remove explicit NULL comparison and write it in its simpler form.
>
>
This would give me confidence in the commit message and log.
And, since I tend toward the obsessive ;), if the next day I do this
fix in another directory, I would repeat this process on that directory
and file because styles can vary by driver/subsystem. Perhaps not on
such a simple fix, but more so as you dive deeper.
alisons
>
> drivers/staging/speakup/fakekey.c | 2 +-
> drivers/staging/speakup/kobjects.c | 2 +-
> drivers/staging/speakup/main.c | 38
> +++---
> 3 files changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/staging/speakup/fakekey.c
> b/drivers/staging/speakup/fakekey.c
> index d76da0a1382c..294c74b47224 100644
> --- a/drivers/staging/speakup/fakekey.c
> +++ b/drivers/staging/speakup/fakekey.c
> @@ -56,7 +56,7 @@ int speakup_add_virtual_keyboard(void)
>
> void speakup_remove_virtual_keyboard(void)
> {
> - if (virt_keyboard != NULL) {
> + if (virt_keyboard) {
> input_unregister_device(virt_keyboard);
> virt_keyboard = NULL;
> }
> diff --git a/drivers/staging/speakup/kobjects.c
> b/drivers/staging/speakup/kobjects.c
> index 5d871ec3693c..2fef55569bfd 100644
> --- a/drivers/staging/speakup/kobjects.c
> +++ b/drivers/staging/speakup/kobjects.c
> @@ -391,7 +391,7 @@ static ssize_t synth_store(struct kobject *kobj, struct
> kobj_attribute *attr,
> len--;
> new_synth_name[len] = '\0';
> spk_strlwr(new_synth_name);
> - if ((synth != NULL) && (!strcmp(new_synth_name, synth->name))) {
> + if (synth && !strcmp(new_synth_name, synth->name)) {
> pr_warn("%s already in use\n", new_synth_name);
> } else if (synth_init(new_synth_name) != 0) {
> pr_warn("failed to init synth %s\n", new_synth_name);
> diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
> index c2f70ef5b9b3..a12ec2b061fe 100644
> --- a/drivers/staging/speakup/main.c
> +++ b/drivers/staging/speakup/main.c
> @@ -299,7 +299,7 @@ static void speakup_shut_up(struct vc_data *vc)
> spk_shut_up |= 0x01;
> spk_parked &= 0xfe;
> speakup_date(vc);
> - if (synth != NULL)
> + if (synth)
> spk_do_flush();
> }
>
> @@ -441,7 +441,7 @@ static void speak_char(u_char ch)
> synth_printf("%s", spk_str_caps_stop);
> return;
> }
> - if (cp == NULL) {
> + if (!cp) {
> pr_info("speak_char: cp == NULL!\n");
> return;
> }
> @@ -1157,7 +1157,7 @@ static void do_handle_shift(struct vc_data *vc, u_char
> value, char up_flag)
> {
> unsigned long flags;
>
> - if (synth == NULL || up_flag || spk_killed)
> + if (!synth || up_flag || spk_killed)
> return;
> spin_lock_irqsave(&speakup_info.spinlock, flags);
> if (cursor_track == read_all_mode) {
> @@ -1195,7 +1195,7 @@ static void do_handle_latin(struct vc_data *vc, u_char
> value, char up_flag)
> spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> return;
> }
> - if (synth == NULL || spk_killed) {
> + if (!synth || spk_killed) {
> spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> return;
> }
> @@ -1279,7 +1279,7 @@ void spk_reset_def
