* [lm-sensors] [PATCH 2/2] lm90.c: fix checkpatch error
@ 2012-01-04 22:27 Frans Meulenbroeks
2012-01-04 22:29 ` Guenter Roeck
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Frans Meulenbroeks @ 2012-01-04 22:27 UTC (permalink / raw)
To: lm-sensors
Signed-off-by: Frans Meulenbroeks <fransmeulenbroeks@gmail.com>
---
drivers/hwmon/lm90.c | 11 ++++++++---
1 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 615bc4f..36dcbc9 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -388,9 +388,14 @@ static int lm90_read16(struct i2c_client *client, u8 regh, u8 regl, u16 *value)
* we have to read the low byte again, and now we believe we have a
* correct reading.
*/
- if ((err = lm90_read_reg(client, regh, &oldh))
- || (err = lm90_read_reg(client, regl, &l))
- || (err = lm90_read_reg(client, regh, &newh)))
+ err = lm90_read_reg(client, regh, &oldh);
+ if (err)
+ return err;
+ err = lm90_read_reg(client, regl, &l);
+ if (err)
+ return err;
+ err = lm90_read_reg(client, regh, &newh);
+ if (err)
return err;
if (oldh != newh) {
err = lm90_read_reg(client, regl, &l);
--
1.7.8.1
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [lm-sensors] [PATCH 2/2] lm90.c: fix checkpatch error
2012-01-04 22:27 [lm-sensors] [PATCH 2/2] lm90.c: fix checkpatch error Frans Meulenbroeks
@ 2012-01-04 22:29 ` Guenter Roeck
2012-01-05 14:29 ` Jean Delvare
2012-01-05 14:45 ` Guenter Roeck
2 siblings, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2012-01-04 22:29 UTC (permalink / raw)
To: lm-sensors
On Wed, 2012-01-04 at 17:27 -0500, Frans Meulenbroeks wrote:
> Signed-off-by: Frans Meulenbroeks <fransmeulenbroeks@gmail.com>
> ---
> drivers/hwmon/lm90.c | 11 ++++++++---
> 1 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> index 615bc4f..36dcbc9 100644
> --- a/drivers/hwmon/lm90.c
> +++ b/drivers/hwmon/lm90.c
> @@ -388,9 +388,14 @@ static int lm90_read16(struct i2c_client *client, u8 regh, u8 regl, u16 *value)
> * we have to read the low byte again, and now we believe we have a
> * correct reading.
> */
> - if ((err = lm90_read_reg(client, regh, &oldh))
> - || (err = lm90_read_reg(client, regl, &l))
> - || (err = lm90_read_reg(client, regh, &newh)))
> + err = lm90_read_reg(client, regh, &oldh);
> + if (err)
> + return err;
> + err = lm90_read_reg(client, regl, &l);
> + if (err)
> + return err;
> + err = lm90_read_reg(client, regh, &newh);
> + if (err)
> return err;
> if (oldh != newh) {
> err = lm90_read_reg(client, regl, &l);
Ah, we left those in for readability.
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [lm-sensors] [PATCH 2/2] lm90.c: fix checkpatch error
2012-01-04 22:27 [lm-sensors] [PATCH 2/2] lm90.c: fix checkpatch error Frans Meulenbroeks
2012-01-04 22:29 ` Guenter Roeck
@ 2012-01-05 14:29 ` Jean Delvare
2012-01-05 14:45 ` Guenter Roeck
2 siblings, 0 replies; 4+ messages in thread
From: Jean Delvare @ 2012-01-05 14:29 UTC (permalink / raw)
To: lm-sensors
On Wed, 4 Jan 2012 14:29:05 -0800, Guenter Roeck wrote:
> On Wed, 2012-01-04 at 17:27 -0500, Frans Meulenbroeks wrote:
> > Signed-off-by: Frans Meulenbroeks <fransmeulenbroeks@gmail.com>
> > ---
> > drivers/hwmon/lm90.c | 11 ++++++++---
> > 1 files changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> > index 615bc4f..36dcbc9 100644
> > --- a/drivers/hwmon/lm90.c
> > +++ b/drivers/hwmon/lm90.c
> > @@ -388,9 +388,14 @@ static int lm90_read16(struct i2c_client *client, u8 regh, u8 regl, u16 *value)
> > * we have to read the low byte again, and now we believe we have a
> > * correct reading.
> > */
> > - if ((err = lm90_read_reg(client, regh, &oldh))
> > - || (err = lm90_read_reg(client, regl, &l))
> > - || (err = lm90_read_reg(client, regh, &newh)))
> > + err = lm90_read_reg(client, regh, &oldh);
> > + if (err)
> > + return err;
> > + err = lm90_read_reg(client, regl, &l);
> > + if (err)
> > + return err;
> > + err = lm90_read_reg(client, regh, &newh);
> > + if (err)
> > return err;
> > if (oldh != newh) {
> > err = lm90_read_reg(client, regl, &l);
>
> Ah, we left those in for readability.
Actually this is one of the things checkpatch complains about which I
consider a false positive. There is really nothing wrong with the code
above, it does the right thing with no risk of error. Its replacement,
OTOH, is harder to read, and less efficient.
The check should either be improved to only complain in cases which are
really wrong, or should be plain discarded if fixing it is not possible.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [lm-sensors] [PATCH 2/2] lm90.c: fix checkpatch error
2012-01-04 22:27 [lm-sensors] [PATCH 2/2] lm90.c: fix checkpatch error Frans Meulenbroeks
2012-01-04 22:29 ` Guenter Roeck
2012-01-05 14:29 ` Jean Delvare
@ 2012-01-05 14:45 ` Guenter Roeck
2 siblings, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2012-01-05 14:45 UTC (permalink / raw)
To: lm-sensors
On Thu, Jan 05, 2012 at 09:29:04AM -0500, Jean Delvare wrote:
> On Wed, 4 Jan 2012 14:29:05 -0800, Guenter Roeck wrote:
> > On Wed, 2012-01-04 at 17:27 -0500, Frans Meulenbroeks wrote:
> > > Signed-off-by: Frans Meulenbroeks <fransmeulenbroeks@gmail.com>
> > > ---
> > > drivers/hwmon/lm90.c | 11 ++++++++---
> > > 1 files changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
> > > index 615bc4f..36dcbc9 100644
> > > --- a/drivers/hwmon/lm90.c
> > > +++ b/drivers/hwmon/lm90.c
> > > @@ -388,9 +388,14 @@ static int lm90_read16(struct i2c_client *client, u8 regh, u8 regl, u16 *value)
> > > * we have to read the low byte again, and now we believe we have a
> > > * correct reading.
> > > */
> > > - if ((err = lm90_read_reg(client, regh, &oldh))
> > > - || (err = lm90_read_reg(client, regl, &l))
> > > - || (err = lm90_read_reg(client, regh, &newh)))
> > > + err = lm90_read_reg(client, regh, &oldh);
> > > + if (err)
> > > + return err;
> > > + err = lm90_read_reg(client, regl, &l);
> > > + if (err)
> > > + return err;
> > > + err = lm90_read_reg(client, regh, &newh);
> > > + if (err)
> > > return err;
> > > if (oldh != newh) {
> > > err = lm90_read_reg(client, regl, &l);
> >
> > Ah, we left those in for readability.
>
> Actually this is one of the things checkpatch complains about which I
> consider a false positive. There is really nothing wrong with the code
> above, it does the right thing with no risk of error. Its replacement,
> OTOH, is harder to read, and less efficient.
>
> The check should either be improved to only complain in cases which are
> really wrong, or should be plain discarded if fixing it is not possible.
>
Strictly speaking, it violates "Don't put multiple statements on a single line",
at least for the first line, so I guess that would be arguable. But I agree, in the
above case the "correct" way hurts readability.
Sad but true, while I personally like to use assignments in if statements
outside the kernel, I have often seen people make a mess of it, so I can
understand why it is frowned upon in the kernel.
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-01-05 14:45 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-01-04 22:27 [lm-sensors] [PATCH 2/2] lm90.c: fix checkpatch error Frans Meulenbroeks
2012-01-04 22:29 ` Guenter Roeck
2012-01-05 14:29 ` Jean Delvare
2012-01-05 14:45 ` Guenter Roeck
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.