Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1490870 > unrolled thread

Re: [PATCH 5/5] ISDN-CAPI: Delete unnecessary braces

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 5/5] ISDN-CAPI: Delete unnecessary braces Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-09-25 13:20 +0200
    Re: [PATCH 5/5] ISDN-CAPI: Delete unnecessary braces SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-25 14:50 +0200
      Re: [PATCH 5/5] ISDN-CAPI: Delete unnecessary braces Paul Bolle <pebolle@tiscali.nl> - 2016-09-26 11:30 +0200
        Re: [PATCH 5/5] ISDN-CAPI: Delete unnecessary braces SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-26 15:00 +0200
          Re: [PATCH 5/5] ISDN-CAPI: Delete unnecessary braces Paul Bolle <pebolle@tiscali.nl> - 2016-09-26 22:00 +0200

#1490870 — Re: [PATCH 5/5] ISDN-CAPI: Delete unnecessary braces

Hello.

On 9/25/2016 2:15 PM, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 25 Sep 2016 12:50:21 +0200
>
> Do not use curly brackets at eight source code places
> where a single statement should be sufficient.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/isdn/capi/capidrv.c | 30 +++++++++++-------------------
>  1 file changed, 11 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/isdn/capi/capidrv.c b/drivers/isdn/capi/capidrv.c
> index 83f756d..7f58644 100644
> --- a/drivers/isdn/capi/capidrv.c
> +++ b/drivers/isdn/capi/capidrv.c
[...]
> @@ -976,13 +974,12 @@ static void handle_controller(_cmsg *cmsg)
>  		if (debugmode)
>  			printk(KERN_DEBUG "capidrv-%d: listenconf Info=0x%4x (%s) cipmask=0x%x\n",
>  			       card->contrnr, cmsg->Info, capi_info2str(cmsg->Info), card->cipmask);
> -		if (cmsg->Info) {
> +		if (cmsg->Info)
>  			listen_change_state(card, EV_LISTEN_CONF_ERROR);
> -		} else if (card->cipmask == 0) {
> +		else if (card->cipmask == 0)
>  			listen_change_state(card, EV_LISTEN_CONF_EMPTY);
> -		} else {
> +		     else

    Indented too much.

[...]

MBR, Sergei

[toc] | [next] | [standalone]

#1490886

>> @@ -976,13 +974,12 @@ static void handle_controller(_cmsg *cmsg)
>>          if (debugmode)
>>              printk(KERN_DEBUG "capidrv-%d: listenconf Info=0x%4x (%s) cipmask=0x%x\n",
>>                     card->contrnr, cmsg->Info, capi_info2str(cmsg->Info), card->cipmask);
>> -        if (cmsg->Info) {
>> +        if (cmsg->Info)
>>              listen_change_state(card, EV_LISTEN_CONF_ERROR);
>> -        } else if (card->cipmask == 0) {
>> +        else if (card->cipmask == 0)
>>              listen_change_state(card, EV_LISTEN_CONF_EMPTY);
>> -        } else {
>> +             else
> 
>    Indented too much.

How do you think about an alignment of this "else"
with the corresponding if statement three lines above?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1491178

On Sun, 2016-09-25 at 14:47 +0200, SF Markus Elfring wrote:
> > > @@ -976,13 +974,12 @@ static void handle_controller(_cmsg *cmsg)
> > >          if (debugmode)
> > >              printk(KERN_DEBUG "capidrv-%d: listenconf Info=0x%4x
> > > (%s) cipmask=0x%x\n",
> > >                     card->contrnr, cmsg->Info,
> > > capi_info2str(cmsg->Info), card->cipmask);
> > > -        if (cmsg->Info) {
> > > +        if (cmsg->Info)
> > >              listen_change_state(card, EV_LISTEN_CONF_ERROR);
> > > -        } else if (card->cipmask == 0) {
> > > +        else if (card->cipmask == 0)
> > >              listen_change_state(card, EV_LISTEN_CONF_EMPTY);
> > > -        } else {
> > > +             else
> > 
> >    Indented too much.
> 
> How do you think about an alignment of this "else"
> with the corresponding if statement three lines above?

Well, I think it looks silly. checkpatch apparently agrees:
    WARNING: Statements should start on a tabstop
    #51: FILE: drivers/isdn/capi/capidrv.c:981:
    +		     else
    
    total: 0 errors, 1 warnings, 91 lines checked
    
    NOTE: For some of the reported defects, checkpatch may be able to
          mechanically convert to the typical style using --fix or --fix-inplace.
    
    Your patch has style problems, please review.
    
    NOTE: If any of the errors are false positives, please report
          them to the maintainer, see CHECKPATCH in MAINTAINERS.

You use checkpatch a lot, don't you? Didn't you use it to, you know,
check your patch?


Paul Bolle

[toc] | [prev] | [next] | [standalone]

#1491288

>>>> @@ -976,13 +974,12 @@ static void handle_controller(_cmsg *cmsg)
>>>>          if (debugmode)
>>>>              printk(KERN_DEBUG "capidrv-%d: listenconf Info=0x%4x
>>>> (%s) cipmask=0x%x\n",
>>>>                     card->contrnr, cmsg->Info,
>>>> capi_info2str(cmsg->Info), card->cipmask);
>>>> -        if (cmsg->Info) {
>>>> +        if (cmsg->Info)
>>>>              listen_change_state(card, EV_LISTEN_CONF_ERROR);
>>>> -        } else if (card->cipmask == 0) {
>>>> +        else if (card->cipmask == 0)
>>>>              listen_change_state(card, EV_LISTEN_CONF_EMPTY);
>>>> -        } else {
>>>> +             else
>>>
>>>    Indented too much.
>>
>> How do you think about an alignment of this "else"
>> with the corresponding if statement three lines above?
> 
> Well, I think it looks silly.

Thanks for your feedback.


> checkpatch apparently agrees:
>     WARNING: Statements should start on a tabstop
>     #51: FILE: drivers/isdn/capi/capidrv.c:981:
>     +		     else
>     
>     total: 0 errors, 1 warnings, 91 lines checked
…
> You use checkpatch a lot, don't you?

It seems so when I am preparing hundreds of update steps.


> Didn't you use it to, you know, check your patch?

This Perl script showed me also the quoted information.

I dared to present a deviation from this general advice
because I got the mood to gather constructive comments on
source code formatting also around an if statement like
in my update suggestion.

Would it eventually make sense to move this "if" (behind an "else")
to a separate line and increase the indentation for the corresponding
code one level then?

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1491531

On Mon, 2016-09-26 at 14:52 +0200, SF Markus Elfring wrote:
> Would it eventually make sense to move this "if" (behind an "else")
> to a separate line and increase the indentation for the corresponding
> code one level then?

No.


Paul Bolle

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web