[PATCH] Staging: greybus: usb: Fixed a coding style error

[PATCH] Staging: greybus: usb: Fixed a coding style error

[Will Cunningham]

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

Re: [PATCH] Staging: greybus: usb: Fixed a coding style error

[Joe Perches]

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.

Re: [greybus-dev] [PATCH] Staging: greybus: usb: Fixed a coding style error

[Alex Elder]

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
> 

Re: [greybus-dev] [PATCH] Staging: greybus: usb: Fixed a coding style error

[Joe Perches]

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.

Re: [greybus-dev] [PATCH] Staging: greybus: usb: Fixed a coding style error

[Alex Elder]

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

Re: [PATCH] Staging: greybus: usb: Fixed a coding style error

[Johan Hovold]

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