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@xxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
