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

From: Alex Elder
Date: Sun Mar 31 2019 - 02:43:27 EST


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