All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Staging: greybus: usb: Fixed a coding style error
@ 2019-03-31  5:30 Will Cunningham
  2019-03-31  6:04 ` Joe Perches
  2019-04-15 12:42 ` Johan Hovold
  0 siblings, 2 replies; 6+ messages in thread
From: Will Cunningham @ 2019-03-31  5:30 UTC (permalink / raw)
  To: johan; +Cc: elder, gregkh, greybus-dev, devel, linux-kernel

Line was >80 characters.

Signed-off-by: Will Cunningham <wjcunningham7@gmail.com>
---
 drivers/staging/greybus/usb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/greybus/usb.c b/drivers/staging/greybus/usb.c
index 1c246c73a085..5b4cbec88159 100644
--- a/drivers/staging/greybus/usb.c
+++ b/drivers/staging/greybus/usb.c
@@ -169,8 +169,8 @@ static int gb_usb_probe(struct gbphy_device *gbphy_dev,
 		return -ENOMEM;
 
 	connection = gb_connection_create(gbphy_dev->bundle,
-					  le16_to_cpu(gbphy_dev->cport_desc->id),
-					  NULL);
+				le16_to_cpu(gbphy_dev->cport_desc->id),
+				NULL);
 	if (IS_ERR(connection)) {
 		retval = PTR_ERR(connection);
 		goto exit_usb_put;
-- 
2.19.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] Staging: greybus: usb: Fixed a coding style error
  2019-03-31  5:30 [PATCH] Staging: greybus: usb: Fixed a coding style error Will Cunningham
@ 2019-03-31  6:04 ` Joe Perches
  2019-03-31  6:20   ` [greybus-dev] " Alex Elder
  2019-04-15 12:42 ` Johan Hovold
  1 sibling, 1 reply; 6+ messages in thread
From: Joe Perches @ 2019-03-31  6:04 UTC (permalink / raw)
  To: Will Cunningham, johan; +Cc: elder, gregkh, greybus-dev, devel, linux-kernel

On Sun, 2019-03-31 at 01:30 -0400, Will Cunningham wrote:
> Line was >80 characters.
[]
> diff --git a/drivers/staging/greybus/usb.c b/drivers/staging/greybus/usb.c
[]
> @@ -169,8 +169,8 @@ static int gb_usb_probe(struct gbphy_device *gbphy_dev,
>  		return -ENOMEM;
>  
>  	connection = gb_connection_create(gbphy_dev->bundle,
> -					  le16_to_cpu(gbphy_dev->cport_desc->id),
> -					  NULL);
> +				le16_to_cpu(gbphy_dev->cport_desc->id),
> +				NULL);

Blind adherence to 80 column limits leads to poor looking
code.  Especially with longish identifier lengths.

Another way to do this, which is not necessarily actually
better is to position the left side of the assignment on a
separate line like:

	connection =
		 gb_connection_create(gbphy_dev->bundle,
				      le16_to_cpu(gbphy_dev->cport_desc->id),

Is that better?  <shrug>  I prefer the original.
It was better before as it was aligned to open parenthesis.



^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [greybus-dev] [PATCH] Staging: greybus: usb: Fixed a coding style error
  2019-03-31  6:04 ` Joe Perches
@ 2019-03-31  6:20   ` Alex Elder
  2019-03-31  6:40     ` Joe Perches
  0 siblings, 1 reply; 6+ messages in thread
From: Alex Elder @ 2019-03-31  6:20 UTC (permalink / raw)
  To: Joe Perches, Will Cunningham, johan
  Cc: devel, elder, linux-kernel, greybus-dev

On 3/31/19 1:04 AM, Joe Perches wrote:
> On Sun, 2019-03-31 at 01:30 -0400, Will Cunningham wrote:
>> Line was >80 characters.
> []
>> diff --git a/drivers/staging/greybus/usb.c b/drivers/staging/greybus/usb.c
> []
>> @@ -169,8 +169,8 @@ static int gb_usb_probe(struct gbphy_device *gbphy_dev,
>>  		return -ENOMEM;
>>  
>>  	connection = gb_connection_create(gbphy_dev->bundle,
>> -					  le16_to_cpu(gbphy_dev->cport_desc->id),
>> -					  NULL);
>> +				le16_to_cpu(gbphy_dev->cport_desc->id),
>> +				NULL);
> 
> Blind adherence to 80 column limits leads to poor looking
> code.  Especially with longish identifier lengths.

I agree.  If it were me, I'd use a local variable.  For example:

        struct greybus_descriptor_cport *cport_desc = gbphy_dev->cport_desc;

	...

        connection = gb_connection_create(gbphy_dev->bundle,
                                          le16_to_cpu(cport_desc->id), NULL);

Or maybe better:

	u16 cport_id = le16_to_cpu(gbphy_dev->cport_desc->id);

	...

        connection = gb_connection_create(gbphy_dev->bundle, cport_id, NULL);

					-Alex




> 
> Another way to do this, which is not necessarily actually
> better is to position the left side of the assignment on a
> separate line like:
> 
> 	connection =
> 		 gb_connection_create(gbphy_dev->bundle,
> 				      le16_to_cpu(gbphy_dev->cport_desc->id),
> 
> Is that better?  <shrug>  I prefer the original.
> It was better before as it was aligned to open parenthesis.
> 
> 
> _______________________________________________
> greybus-dev mailing list
> greybus-dev@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/greybus-dev
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [greybus-dev] [PATCH] Staging: greybus: usb: Fixed a coding style error
  2019-03-31  6:20   ` [greybus-dev] " Alex Elder
@ 2019-03-31  6:40     ` Joe Perches
  2019-03-31  6:43       ` Alex Elder
  0 siblings, 1 reply; 6+ messages in thread
From: Joe Perches @ 2019-03-31  6:40 UTC (permalink / raw)
  To: Alex Elder, Will Cunningham, johan
  Cc: devel, elder, linux-kernel, greybus-dev

On Sun, 2019-03-31 at 01:20 -0500, Alex Elder wrote:
> On 3/31/19 1:04 AM, Joe Perches wrote:
> > Blind adherence to 80 column limits leads to poor looking
> > code.  Especially with longish identifier lengths.
> I agree.  If it were me, I'd use a local variable.  For example:
> 
> 	struct greybus_descriptor_cport *cport_desc = gbphy_dev->cport_desc;
> 	...
> 	connection = gb_connection_create(gbphy_dev->bundle,
> 					  le16_to_cpu(cport_desc->id), NULL);
> 
> Or maybe better:
> 
> 	u16 cport_id = le16_to_cpu(gbphy_dev->cport_desc->id);
> 	...
> 	connection = gb_connection_create(gbphy_dev->bundle, cport_id, NULL);

True.

A possible negative though:

Temporaries that are only used once are sometimes
less readable as the declaration is supposed to be
done at an open brace and that could be relatively
far away from the set and use.


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [greybus-dev] [PATCH] Staging: greybus: usb: Fixed a coding style error
  2019-03-31  6:40     ` Joe Perches
@ 2019-03-31  6:43       ` Alex Elder
  0 siblings, 0 replies; 6+ messages in thread
From: Alex Elder @ 2019-03-31  6:43 UTC (permalink / raw)
  To: Joe Perches, Will Cunningham, johan
  Cc: devel, elder, linux-kernel, greybus-dev

On 3/31/19 1:40 AM, Joe Perches wrote:
> On Sun, 2019-03-31 at 01:20 -0500, Alex Elder wrote:
>> On 3/31/19 1:04 AM, Joe Perches wrote:
>>> Blind adherence to 80 column limits leads to poor looking
>>> code.  Especially with longish identifier lengths.
>> I agree.  If it were me, I'd use a local variable.  For example:
>>
>> 	struct greybus_descriptor_cport *cport_desc = gbphy_dev->cport_desc;
>> 	...
>> 	connection = gb_connection_create(gbphy_dev->bundle,
>> 					  le16_to_cpu(cport_desc->id), NULL);
>>
>> Or maybe better:
>>
>> 	u16 cport_id = le16_to_cpu(gbphy_dev->cport_desc->id);
>> 	...
>> 	connection = gb_connection_create(gbphy_dev->bundle, cport_id, NULL);
> 
> True.
> 
> A possible negative though:
> 
> Temporaries that are only used once are sometimes
> less readable as the declaration is supposed to be
> done at an open brace and that could be relatively
> far away from the set and use.

Then assign it where it's used.  The point is we're talking about
a readability issue (long lines), and no matter how you try to fix
it there are tradeoffs, and it's subjective.  In any case, I prefer
the use of the local variable to solve this readability problem over
splitting the line.

					-Alex


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] Staging: greybus: usb: Fixed a coding style error
  2019-03-31  5:30 [PATCH] Staging: greybus: usb: Fixed a coding style error Will Cunningham
  2019-03-31  6:04 ` Joe Perches
@ 2019-04-15 12:42 ` Johan Hovold
  1 sibling, 0 replies; 6+ messages in thread
From: Johan Hovold @ 2019-04-15 12:42 UTC (permalink / raw)
  To: Will Cunningham; +Cc: johan, elder, gregkh, greybus-dev, devel, linux-kernel

On Sun, Mar 31, 2019 at 01:30:40AM -0400, Will Cunningham wrote:
> Line was >80 characters.
> 
> Signed-off-by: Will Cunningham <wjcunningham7@gmail.com>
> ---
>  drivers/staging/greybus/usb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/greybus/usb.c b/drivers/staging/greybus/usb.c
> index 1c246c73a085..5b4cbec88159 100644
> --- a/drivers/staging/greybus/usb.c
> +++ b/drivers/staging/greybus/usb.c
> @@ -169,8 +169,8 @@ static int gb_usb_probe(struct gbphy_device *gbphy_dev,
>  		return -ENOMEM;
>  
>  	connection = gb_connection_create(gbphy_dev->bundle,
> -					  le16_to_cpu(gbphy_dev->cport_desc->id),
> -					  NULL);
> +				le16_to_cpu(gbphy_dev->cport_desc->id),
> +				NULL);

As others have already pointed out in this thread, there's no need to
fix anything here.

The 80 column rule is not absolute in any way, and having a line be 81
characters if that improves readability is just fine.

Johan

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-04-15 12:42 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-31  5:30 [PATCH] Staging: greybus: usb: Fixed a coding style error Will Cunningham
2019-03-31  6:04 ` Joe Perches
2019-03-31  6:20   ` [greybus-dev] " Alex Elder
2019-03-31  6:40     ` Joe Perches
2019-03-31  6:43       ` Alex Elder
2019-04-15 12:42 ` Johan Hovold

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.