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;
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.
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
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.
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
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
-
Alex Elder -
Joe Perches -
Johan Hovold -
Will Cunningham