checkpatch: do not warn for multiline parenthesized returned value

[Qemu-devel] [PATCH RFC] checkpatch: do not warn for multiline parenthesized returned value

Posted by Paolo Bonzini 6 years, 4 months ago

While indeed we do not want to have

    return (a);

it is less clear that this applies to

    return (a &&
            b);

Some editors indent more nicely if you have parentheses, and some people's
eyes may appreciate that as well.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/checkpatch.pl | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c2aaf42..2f81371 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2296,7 +2296,8 @@ sub process {
 			       $value =~ s/\([^\(\)]*\)/1/) {
 			}
 #print "value<$value>\n";
-			if ($value =~ /^\s*(?:$Ident|-?$Constant)\s*$/) {
+			if ($value =~ /^\s*(?:$Ident|-?$Constant)\s*$/ &&
+			    $line =~ /;$/) {
 				ERROR("return is not a function, parentheses are not required\n" . $herecurr);
 
 			} elsif ($spacing !~ /\s+/) {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH RFC] checkpatch: do not warn for multiline parenthesized returned value

Posted by Eric Blake 6 years, 4 months ago

On 6/21/19 6:28 AM, Paolo Bonzini wrote:
> While indeed we do not want to have
> 
>     return (a);
> 
> it is less clear that this applies to
> 
>     return (a &&
>             b);
> 
> Some editors indent more nicely if you have parentheses, and some people's
> eyes may appreciate that as well.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  scripts/checkpatch.pl | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

I'm certainly in favor of this (as I've been known to use this style).

> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index c2aaf42..2f81371 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2296,7 +2296,8 @@ sub process {
>  			       $value =~ s/\([^\(\)]*\)/1/) {
>  			}
>  #print "value<$value>\n";
> -			if ($value =~ /^\s*(?:$Ident|-?$Constant)\s*$/) {
> +			if ($value =~ /^\s*(?:$Ident|-?$Constant)\s*$/ &&
> +			    $line =~ /;$/) {

So the diagnosis now checks for a trailing ';' as its witness of whether
this is a one-liner return statement, leaving multi-liners undiagnosed.
Easy enough to understand.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH RFC] checkpatch: do not warn for multiline parenthesized returned value

Posted by Markus Armbruster 6 years, 4 months ago

Paolo Bonzini <pbonzini@redhat.com> writes:

> While indeed we do not want to have
>
>     return (a);
>
> it is less clear that this applies to
>
>     return (a &&
>             b);
>
> Some editors indent more nicely if you have parentheses, and some people's
> eyes may appreciate that as well.

No objection.

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  scripts/checkpatch.pl | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index c2aaf42..2f81371 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2296,7 +2296,8 @@ sub process {
>  			       $value =~ s/\([^\(\)]*\)/1/) {
>  			}
>  #print "value<$value>\n";
> -			if ($value =~ /^\s*(?:$Ident|-?$Constant)\s*$/) {
> +			if ($value =~ /^\s*(?:$Ident|-?$Constant)\s*$/ &&
> +			    $line =~ /;$/) {
>  				ERROR("return is not a function, parentheses are not required\n" . $herecurr);
>  
>  			} elsif ($spacing !~ /\s+/) {

Re: [Qemu-devel] [PATCH RFC] checkpatch: do not warn for multiline parenthesized returned value

Posted by Richard Henderson 6 years, 4 months ago

On 6/21/19 1:28 PM, Paolo Bonzini wrote:
> While indeed we do not want to have
> 
>     return (a);
> 
> it is less clear that this applies to
> 
>     return (a &&
>             b);
> 
> Some editors indent more nicely if you have parentheses, and some people's
> eyes may appreciate that as well.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  scripts/checkpatch.pl | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Thanks, I do this semi-regularly.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~