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

Groups > linux.kernel > #1712738 > unrolled thread

Re: [PATCH v2 1/4] staging: pi433: Style fix - Correct long lines

Started byDan Carpenter <dan.carpenter@oracle.com>
First post2017-08-16 09:40 +0200
Last post2017-08-22 12:50 +0200
Articles 6 — 4 participants

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 v2 1/4] staging: pi433: Style fix - Correct long lines Dan Carpenter <dan.carpenter@oracle.com> - 2017-08-16 09:40 +0200
    Re: [PATCH v2 1/4] staging: pi433: Style fix - Correct long lines Joe Perches <joe@perches.com> - 2017-08-20 06:50 +0200
      Re: [PATCH v2 1/4] staging: pi433: Style fix - Correct long lines Rishabh Hardas <rishabhhardas@gmail.com> - 2017-08-22 11:30 +0200
        Re: [PATCH v2 1/4] staging: pi433: Style fix - Correct long lines Dan Carpenter <dan.carpenter@oracle.com> - 2017-08-22 12:10 +0200
          Re: [PATCH v2 1/4] staging: pi433: Style fix - Correct long lines Marcus Wolf <marcus.wolf@wolf-entwicklungen.de> - 2017-08-22 12:40 +0200
            Re: [PATCH v2 1/4] staging: pi433: Style fix - Correct long lines Dan Carpenter <dan.carpenter@oracle.com> - 2017-08-22 12:50 +0200

#1712738 — Re: [PATCH v2 1/4] staging: pi433: Style fix - Correct long lines

FromDan Carpenter <dan.carpenter@oracle.com>
Date2017-08-16 09:40 +0200
SubjectRe: [PATCH v2 1/4] staging: pi433: Style fix - Correct long lines
Message-ID<uf15M-498-29@gated-at.bofh.it>
On Wed, Aug 16, 2017 at 10:53:18AM +0530, Rishabh Hardas wrote:
> @@ -143,10 +142,13 @@ struct pi433_rx_cfg {
> 
>  #define PI433_IOC_MAGIC			'r'
> 
> -#define PI433_IOC_RD_TX_CFG	_IOR(PI433_IOC_MAGIC, PI433_TX_CFG_IOCTL_NR, char[sizeof(struct pi433_tx_cfg)])
> -#define PI433_IOC_WR_TX_CFG	_IOW(PI433_IOC_MAGIC, PI433_TX_CFG_IOCTL_NR, char[sizeof(struct pi433_tx_cfg)])
> -
> -#define PI433_IOC_RD_RX_CFG	_IOR(PI433_IOC_MAGIC, PI433_RX_CFG_IOCTL_NR, char[sizeof(struct pi433_rx_cfg)])
> -#define PI433_IOC_WR_RX_CFG	_IOW(PI433_IOC_MAGIC, PI433_RX_CFG_IOCTL_NR, char[sizeof(struct pi433_rx_cfg)])
> +#define PI433_IOC_RD_TX_CFG	_IOR(PI433_IOC_MAGIC, PI433_TX_CFG_IOCTL_NR,\
> +				     char[sizeof(struct pi433_tx_cfg)])
> +#define PI433_IOC_WR_TX_CFG	_IOW(PI433_IOC_MAGIC, PI433_TX_CFG_IOCTL_NR,\
> +				     char[sizeof(struct pi433_tx_cfg)])
> +#define PI433_IOC_RD_RX_CFG	_IOR(PI433_IOC_MAGIC, PI433_RX_CFG_IOCTL_NR,\
> +				     char[sizeof(struct pi433_rx_cfg)])
> +#define PI433_IOC_WR_RX_CFG	_IOW(PI433_IOC_MAGIC, PI433_RX_CFG_IOCTL_NR,\
> +				     char[sizeof(struct pi433_rx_cfg)])


These don't help readability.  The original was better.

regards,
dan carpenter

[toc] | [next] | [standalone]

#1715866

FromJoe Perches <joe@perches.com>
Date2017-08-20 06:50 +0200
Message-ID<ugqlr-2f4-5@gated-at.bofh.it>
In reply to#1712738
On Wed, 2017-08-16 at 10:31 +0300, Dan Carpenter wrote:
> On Wed, Aug 16, 2017 at 10:53:18AM +0530, Rishabh Hardas wrote:
> > @@ -143,10 +142,13 @@ struct pi433_rx_cfg {
> > 
> >  #define PI433_IOC_MAGIC			'r'
> > 
> > -#define PI433_IOC_RD_TX_CFG	_IOR(PI433_IOC_MAGIC, PI433_TX_CFG_IOCTL_NR, char[sizeof(struct pi433_tx_cfg)])
> > -#define PI433_IOC_WR_TX_CFG	_IOW(PI433_IOC_MAGIC, PI433_TX_CFG_IOCTL_NR, char[sizeof(struct pi433_tx_cfg)])
> > -
> > -#define PI433_IOC_RD_RX_CFG	_IOR(PI433_IOC_MAGIC, PI433_RX_CFG_IOCTL_NR, char[sizeof(struct pi433_rx_cfg)])
> > -#define PI433_IOC_WR_RX_CFG	_IOW(PI433_IOC_MAGIC, PI433_RX_CFG_IOCTL_NR, char[sizeof(struct pi433_rx_cfg)])
> > +#define PI433_IOC_RD_TX_CFG	_IOR(PI433_IOC_MAGIC, PI433_TX_CFG_IOCTL_NR,\
> > +				     char[sizeof(struct pi433_tx_cfg)])
> > +#define PI433_IOC_WR_TX_CFG	_IOW(PI433_IOC_MAGIC, PI433_TX_CFG_IOCTL_NR,\
> > +				     char[sizeof(struct pi433_tx_cfg)])
> > +#define PI433_IOC_RD_RX_CFG	_IOR(PI433_IOC_MAGIC, PI433_RX_CFG_IOCTL_NR,\
> > +				     char[sizeof(struct pi433_rx_cfg)])
> > +#define PI433_IOC_WR_RX_CFG	_IOW(PI433_IOC_MAGIC, PI433_RX_CFG_IOCTL_NR,\
> > +				     char[sizeof(struct pi433_rx_cfg)])
> 
> 
> These don't help readability.  The original was better.

The original wasn't any good either.

It'd be better to avoid the macros altogether
as almost all are use-once.

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

#1717213

FromRishabh Hardas <rishabhhardas@gmail.com>
Date2017-08-22 11:30 +0200
Message-ID<uhdFv-ng-1@gated-at.bofh.it>
In reply to#1715866
On Sat, Aug 19, 2017 at 09:47:28PM -0700, Joe Perches wrote:
> On Wed, 2017-08-16 at 10:31 +0300, Dan Carpenter wrote:
> > On Wed, Aug 16, 2017 at 10:53:18AM +0530, Rishabh Hardas wrote:
> > > @@ -143,10 +142,13 @@ struct pi433_rx_cfg {
> > > 
> > >  #define PI433_IOC_MAGIC			'r'
> > > 
> > > -#define PI433_IOC_RD_TX_CFG	_IOR(PI433_IOC_MAGIC, PI433_TX_CFG_IOCTL_NR, char[sizeof(struct pi433_tx_cfg)])
> > > -#define PI433_IOC_WR_TX_CFG	_IOW(PI433_IOC_MAGIC, PI433_TX_CFG_IOCTL_NR, char[sizeof(struct pi433_tx_cfg)])
> > > -
> > > -#define PI433_IOC_RD_RX_CFG	_IOR(PI433_IOC_MAGIC, PI433_RX_CFG_IOCTL_NR, char[sizeof(struct pi433_rx_cfg)])
> > > -#define PI433_IOC_WR_RX_CFG	_IOW(PI433_IOC_MAGIC, PI433_RX_CFG_IOCTL_NR, char[sizeof(struct pi433_rx_cfg)])
> > > +#define PI433_IOC_RD_TX_CFG	_IOR(PI433_IOC_MAGIC, PI433_TX_CFG_IOCTL_NR,\
> > > +				     char[sizeof(struct pi433_tx_cfg)])
> > > +#define PI433_IOC_WR_TX_CFG	_IOW(PI433_IOC_MAGIC, PI433_TX_CFG_IOCTL_NR,\
> > > +				     char[sizeof(struct pi433_tx_cfg)])
> > > +#define PI433_IOC_RD_RX_CFG	_IOR(PI433_IOC_MAGIC, PI433_RX_CFG_IOCTL_NR,\
> > > +				     char[sizeof(struct pi433_rx_cfg)])
> > > +#define PI433_IOC_WR_RX_CFG	_IOW(PI433_IOC_MAGIC, PI433_RX_CFG_IOCTL_NR,\
> > > +				     char[sizeof(struct pi433_rx_cfg)])
> > 
> > 
> > These don't help readability.  The original was better.
> 
> The original wasn't any good either.
> 
> It'd be better to avoid the macros altogether
> as almost all are use-once.
> 
> 
So should I keep this as it is or remove the macros ?
Because as Dan said the corrections that I made aren't goo either.

Regards
Rishabh Hardas

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

#1717261

FromDan Carpenter <dan.carpenter@oracle.com>
Date2017-08-22 12:10 +0200
Message-ID<uheie-T0-23@gated-at.bofh.it>
In reply to#1717213
On Tue, Aug 22, 2017 at 02:57:49PM +0530, Rishabh Hardas wrote:
> On Sat, Aug 19, 2017 at 09:47:28PM -0700, Joe Perches wrote:
> > On Wed, 2017-08-16 at 10:31 +0300, Dan Carpenter wrote:
> > > On Wed, Aug 16, 2017 at 10:53:18AM +0530, Rishabh Hardas wrote:
> > > > @@ -143,10 +142,13 @@ struct pi433_rx_cfg {
> > > > 
> > > >  #define PI433_IOC_MAGIC			'r'
> > > > 
> > > > -#define PI433_IOC_RD_TX_CFG	_IOR(PI433_IOC_MAGIC, PI433_TX_CFG_IOCTL_NR, char[sizeof(struct pi433_tx_cfg)])
> > > > -#define PI433_IOC_WR_TX_CFG	_IOW(PI433_IOC_MAGIC, PI433_TX_CFG_IOCTL_NR, char[sizeof(struct pi433_tx_cfg)])
> > > > -
> > > > -#define PI433_IOC_RD_RX_CFG	_IOR(PI433_IOC_MAGIC, PI433_RX_CFG_IOCTL_NR, char[sizeof(struct pi433_rx_cfg)])
> > > > -#define PI433_IOC_WR_RX_CFG	_IOW(PI433_IOC_MAGIC, PI433_RX_CFG_IOCTL_NR, char[sizeof(struct pi433_rx_cfg)])
> > > > +#define PI433_IOC_RD_TX_CFG	_IOR(PI433_IOC_MAGIC, PI433_TX_CFG_IOCTL_NR,\
> > > > +				     char[sizeof(struct pi433_tx_cfg)])
> > > > +#define PI433_IOC_WR_TX_CFG	_IOW(PI433_IOC_MAGIC, PI433_TX_CFG_IOCTL_NR,\
> > > > +				     char[sizeof(struct pi433_tx_cfg)])
> > > > +#define PI433_IOC_RD_RX_CFG	_IOR(PI433_IOC_MAGIC, PI433_RX_CFG_IOCTL_NR,\
> > > > +				     char[sizeof(struct pi433_rx_cfg)])
> > > > +#define PI433_IOC_WR_RX_CFG	_IOW(PI433_IOC_MAGIC, PI433_RX_CFG_IOCTL_NR,\
> > > > +				     char[sizeof(struct pi433_rx_cfg)])
> > > 
> > > 
> > > These don't help readability.  The original was better.
> > 
> > The original wasn't any good either.
> > 
> > It'd be better to avoid the macros altogether
> > as almost all are use-once.
> > 
> > 
> So should I keep this as it is or remove the macros ?
> Because as Dan said the corrections that I made aren't goo either.
> 

Find a way to correct it which makes the code more readable than it was
before.

regards,
dan carpenter

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

#1717293

FromMarcus Wolf <marcus.wolf@wolf-entwicklungen.de>
Date2017-08-22 12:40 +0200
Message-ID<uheLf-14J-19@gated-at.bofh.it>
In reply to#1717261
Hi everybody,

from my point of view, we should stay with the old implementation.

Ok - line is too long according to style guide. But these long lines are 
IMHO easy to read:
All four are pretty similar. By having all Tokens in exact the same length 
and having one below other, you can easily detect the differences between
the lines and that's important. As soon as you start to wrap them 
- regardless how - you won't be able to detect the differences that easy 
any more - and from my Point of view that's a disadvantage.

Cheers,

Marcus

> Dan Carpenter <dan.carpenter@oracle.com> hat am 22. August 2017 um 12:03
> geschrieben:
>
>
> On Tue, Aug 22, 2017 at 02:57:49PM +0530, Rishabh Hardas wrote:
> > On Sat, Aug 19, 2017 at 09:47:28PM -0700, Joe Perches wrote:
> > > On Wed, 2017-08-16 at 10:31 +0300, Dan Carpenter wrote:
> > > > On Wed, Aug 16, 2017 at 10:53:18AM +0530, Rishabh Hardas wrote:
> > > > > @@ -143,10 +142,13 @@ struct pi433_rx_cfg {
> > > > >
> > > > > #define PI433_IOC_MAGIC 'r'
> > > > >
> > > > > -#define PI433_IOC_RD_TX_CFG _IOR(PI433_IOC_MAGIC,
> > > > > PI433_TX_CFG_IOCTL_NR, char[sizeof(struct pi433_tx_cfg)])
> > > > > -#define PI433_IOC_WR_TX_CFG _IOW(PI433_IOC_MAGIC,
> > > > > PI433_TX_CFG_IOCTL_NR, char[sizeof(struct pi433_tx_cfg)])
> > > > > -
> > > > > -#define PI433_IOC_RD_RX_CFG _IOR(PI433_IOC_MAGIC,
> > > > > PI433_RX_CFG_IOCTL_NR, char[sizeof(struct pi433_rx_cfg)])
> > > > > -#define PI433_IOC_WR_RX_CFG _IOW(PI433_IOC_MAGIC,
> > > > > PI433_RX_CFG_IOCTL_NR, char[sizeof(struct pi433_rx_cfg)])
> > > > > +#define PI433_IOC_RD_TX_CFG _IOR(PI433_IOC_MAGIC,
> > > > > PI433_TX_CFG_IOCTL_NR,\
> > > > > + char[sizeof(struct pi433_tx_cfg)])
> > > > > +#define PI433_IOC_WR_TX_CFG _IOW(PI433_IOC_MAGIC,
> > > > > PI433_TX_CFG_IOCTL_NR,\
> > > > > + char[sizeof(struct pi433_tx_cfg)])
> > > > > +#define PI433_IOC_RD_RX_CFG _IOR(PI433_IOC_MAGIC,
> > > > > PI433_RX_CFG_IOCTL_NR,\
> > > > > + char[sizeof(struct pi433_rx_cfg)])
> > > > > +#define PI433_IOC_WR_RX_CFG _IOW(PI433_IOC_MAGIC,
> > > > > PI433_RX_CFG_IOCTL_NR,\
> > > > > + char[sizeof(struct pi433_rx_cfg)])
> > > >
> > > >
> > > > These don't help readability. The original was better.
> > >
> > > The original wasn't any good either.
> > >
> > > It'd be better to avoid the macros altogether
> > > as almost all are use-once.
> > >
> > >
> > So should I keep this as it is or remove the macros ?
> > Because as Dan said the corrections that I made aren't goo either.
> >
>
> Find a way to correct it which makes the code more readable than it was
> before.
>
> regards,
> dan carpenter
>

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

#1717304

FromDan Carpenter <dan.carpenter@oracle.com>
Date2017-08-22 12:50 +0200
Message-ID<uheUW-18s-15@gated-at.bofh.it>
In reply to#1717293
I'm skeptical that these are the absolute platonic ideal of what the
code should look like...  You may be right that it's mathematically
impossible to make these lines more readable, but we can't really debate
alternate versions of this until someone sends us a patch.

regards,
dan carpenter

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web