[PATCH] Staging: speakup: Change else-if block order

[PATCH] Staging: speakup: Change else-if block order

[Wentao Cai]

Change else-if block to silence checkpatch.pl warning:
WARNING: else is not generally useful after a break or return

Signed-off-by: Wentao Cai <etsai042@gmail.com>
---
 drivers/staging/speakup/keyhelp.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/speakup/keyhelp.c b/drivers/staging/speakup/keyhelp.c
index 5f1bda37f86d..23ec8f01e0a2 100644
--- a/drivers/staging/speakup/keyhelp.c
+++ b/drivers/staging/speakup/keyhelp.c
@@ -153,6 +153,12 @@ int spk_handle_help(struct vc_data *vc, u_char type, u_char ch, u_short key)
 			return 1;
 		}
 		cur_item = letter_offsets[ch - 'a'];
+	} else if (type == KT_SPKUP && ch == SPEAKUP_HELP &&
+		   !spk_special_handler) {
+		spk_special_handler = spk_handle_help;
+		synth_printf("%s\n", spk_msg_get(MSG_HELP_INFO));
+		build_key_data(); /* rebuild each time in case new mapping */
+		return 1;
 	} else if (type == KT_CUR) {
 		if (ch == 0 &&
 		    (MSG_FUNCNAMES_START + cur_item + 1) <= MSG_FUNCNAMES_END)
@@ -161,12 +167,6 @@ int spk_handle_help(struct vc_data *vc, u_char type, u_char ch, u_short key)
 			cur_item--;
 		else
 			return -1;
-	} else if (type == KT_SPKUP && ch == SPEAKUP_HELP &&
-		   !spk_special_handler) {
-		spk_special_handler = spk_handle_help;
-		synth_printf("%s\n", spk_msg_get(MSG_HELP_INFO));
-		build_key_data(); /* rebuild each time in case new mapping */
-		return 1;
 	} else {
 		name = NULL;
 		if ((type != KT_SPKUP) && (key > 0) && (key <= num_key_names)) {
-- 
2.11.0

Re: [PATCH] Staging: speakup: Change else-if block order

[Samuel Thibault]

Wentao Cai, le mar. 12 mars 2019 02:15:12 -0700, a ecrit:
> Change else-if block to silence checkpatch.pl warning:
> WARNING: else is not generally useful after a break or return

This looks like a false positive of checkpatch.pl, I don't see the point
of moving this else if block.

Samuel

Re: [Outreachy kernel] [PATCH] Staging: speakup: Change else-if block order

[Julia Lawall]

On Tue, 12 Mar 2019, Wentao Cai wrote:

> Change else-if block to silence checkpatch.pl warning:
> WARNING: else is not generally useful after a break or return
>
> Signed-off-by: Wentao Cai <etsai042@gmail.com>
> ---
>  drivers/staging/speakup/keyhelp.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/speakup/keyhelp.c b/drivers/staging/speakup/keyhelp.c
> index 5f1bda37f86d..23ec8f01e0a2 100644
> --- a/drivers/staging/speakup/keyhelp.c
> +++ b/drivers/staging/speakup/keyhelp.c
> @@ -153,6 +153,12 @@ int spk_handle_help(struct vc_data *vc, u_char type, u_char ch, u_short key)
>  			return 1;
>  		}
>  		cur_item = letter_offsets[ch - 'a'];
> +	} else if (type == KT_SPKUP && ch == SPEAKUP_HELP &&
> +		   !spk_special_handler) {
> +		spk_special_handler = spk_handle_help;
> +		synth_printf("%s\n", spk_msg_get(MSG_HELP_INFO));
> +		build_key_data(); /* rebuild each time in case new mapping */
> +		return 1;
>  	} else if (type == KT_CUR) {
>  		if (ch == 0 &&
>  		    (MSG_FUNCNAMES_START + cur_item + 1) <= MSG_FUNCNAMES_END)
> @@ -161,12 +167,6 @@ int spk_handle_help(struct vc_data *vc, u_char type, u_char ch, u_short key)
>  			cur_item--;
>  		else
>  			return -1;
> -	} else if (type == KT_SPKUP && ch == SPEAKUP_HELP &&
> -		   !spk_special_handler) {
> -		spk_special_handler = spk_handle_help;
> -		synth_printf("%s\n", spk_msg_get(MSG_HELP_INFO));
> -		build_key_data(); /* rebuild each time in case new mapping */
> -		return 1;

This would definitely be tha case of silencing a warning, because the
warning appears to be a false positive.  There is a return on the previous
line, but it is under an else, so it's not guaranteed to occur.

Furthermoew, by making this change, I guess you obtain another occurrence
of this warning, becaue your added code ends with a return and the next
line contains an else.

If this really seems to be an improvement, the log message needs to
explain why moving the if around preserves the semantics of the code.  Try
to avoid log messages that just say Change or Fix.  Those words don't give
any information about what you did or why it is useful.

julia

>  	} else {
>  		name = NULL;
>  		if ((type != KT_SPKUP) && (key > 0) && (key <= num_key_names)) {
> --
> 2.11.0
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20190312091512.2315-1-etsai042%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH] Staging: speakup: Change else-if block order

[etsai042]

[-- Attachment #1.1: Type: text/plain, Size: 2267 bytes --]



On Tuesday, March 12, 2019 at 3:16:26 AM UTC-7, Julia Lawall wrote:
>
>
> This would definitely be tha case of silencing a warning, because the 
> warning appears to be a false positive.  There is a return on the previous 
> line, but it is under an else, so it's not guaranteed to occur. 
>
> Furthermoew, by making this change, I guess you obtain another occurrence 
> of this warning, becaue your added code ends with a return and the next 
> line contains an else. 
>
> I think the logic of checkpatch.pl is that, else under break or return 
directly is not recommended. But if the return is in a nested else, this 
warning will not appear since there can be other if-block or elseif-block 
behind it that doesn't always return. This change doesn't generate more 
warnings.
In this file we can't simply delete the last else because the if-block 
behind it doesn't return in all cases.
 

> If this really seems to be an improvement, the log message needs to 
> explain why moving the if around preserves the semantics of the code.  Try 
> to avoid log messages that just say Change or Fix.  Those words don't give 
> any information about what you did or why it is useful. 
>
>
Though it silence this warning, I think this change is not necessary. Maybe 
checkpatch.pl should be revised, to check the whole if-else flow control, 
and to make sure to give out this warning only when all blocks behind the 
last else has a return?

Thanks for the log message advice, I won't use Change or Fix next time.

Wentao
 

> julia 
>
> >          } else { 
> >                  name = NULL; 
> >                  if ((type != KT_SPKUP) && (key > 0) && (key <= 
> num_key_names)) { 
> > -- 
> > 2.11.0 
> > 
> > -- 
> > You received this message because you are subscribed to the Google 
> Groups "outreachy-kernel" group. 
> > To unsubscribe from this group and stop receiving emails from it, send 
> an email to outreachy-kern...@googlegroups.com <javascript:>. 
> > To post to this group, send email to outreach...@googlegroups.com 
> <javascript:>. 
> > To view this discussion on the web visit 
> https://groups.google.com/d/msgid/outreachy-kernel/20190312091512.2315-1-etsai042%40gmail.com. 
>
> > For more options, visit https://groups.google.com/d/optout. 
> > 
>

[-- Attachment #1.2: Type: text/html, Size: 4046 bytes --]

Re: [Outreachy kernel] [PATCH] Staging: speakup: Change else-if block order

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 3288 bytes --]



On Tue, 12 Mar 2019, etsai042@gmail.com wrote:

>
>
> On Tuesday, March 12, 2019 at 3:16:26 AM UTC-7, Julia Lawall wrote:
>
>       This would definitely be tha case of silencing a warning, because the
>       warning appears to be a false positive. ï¿œThere is a return on the previous
>       line, but it is under an else, so it's not guaranteed to occur.
>
>       Furthermoew, by making this change, I guess you obtain another occurrence
>       of this warning, becaue your added code ends with a return and the next
>       line contains an else.
>
> I think the logic of checkpatch.pl is that, else under break or return directly is not recommended. But if the return is in a nested else, this warning will not appear since there can be other if-block or elseif-block behind it that
> doesn't always return. This change doesn't generate more warnings.
> In this file we can't simply delete the last else because the if-block behind it doesn't return in all cases.

As far as I recall from the context, there was an else return above the
removed code.  But perhaps I recall incorrectly.

> ￜ
>       If this really seems to be an improvement, the log message needs to
>       explain why moving the if around preserves the semantics of the code. ï¿œTry
>       to avoid log messages that just say Change or Fix. ï¿œThose words don't give
>       any information about what you did or why it is useful.
>
>
> Though it silence this warning, I think this change is not necessary. Maybe checkpatch.pl should be revised, to check the whole if-else flow control, and to make sure to give out this warning only when all blocks behind the last else
> has a return?

Checkpatch just uses perl regular expressions.  It desn't parse the code.
It doesn't know the boundaries of the whole else branch.

julia

> Thanks for the log message advice, I won't use Change or Fix next time.
>
> Wentao
> ￜ
>       julia
>
>       > ￜￜￜￜￜￜￜￜￜ} else {
>       > ￜￜￜￜￜￜￜￜￜￜￜￜￜￜￜￜￜname = NULL;
>       > ￜￜￜￜￜￜￜￜￜￜￜￜￜￜￜￜￜif ((type != KT_SPKUP) && (key > 0) && (key <= num_key_names)) {
>       > --
>       > 2.11.0
>       >
>       > --
>       > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
>       > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kern...@googlegroups.com.
>       > To post to this group, send email to outreach...@googlegroups.com.
>       > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20190312091512.2315-1-etsai042%40gmail.com.
>       > For more options, visit https://groups.google.com/d/optout.
>       >
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/75c4ca91-0811-4a19-a9e2-29b127d419c9%40googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>
>