| Message ID | Y2iFGA3A1w+XMlYU@qemulion (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | staging: iio: meter: use min() for comparison and assignment | expand |
On Mon, Nov 07, 2022 at 09:40:00AM +0530, Deepak R Varma wrote: > Simplify code by using recommended min helper macro for logical > evaluation and value assignment. This issue is identified by > coccicheck using the minmax.cocci file. > > Signed-off-by: Deepak R Varma <drv@mailo.com> > --- > drivers/staging/iio/meter/ade7854-i2c.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c > index a9a06e8dda51..a6ce7b24cc8f 100644 > --- a/drivers/staging/iio/meter/ade7854-i2c.c > +++ b/drivers/staging/iio/meter/ade7854-i2c.c > @@ -61,7 +61,7 @@ static int ade7854_i2c_write_reg(struct device *dev, > unlock: > mutex_unlock(&st->buf_lock); > > - return ret < 0 ? ret : 0; > + return min(ret, 0); The original code is better. If it's a failure return the error code. If it's not return zero. You can only compare apples to apples. min() makes sense if you're talking about two lengths. But here if ret is negative that's an error code. If it's positive that's the number of bytes. If the error code is less than the number of bytes then return that? What??? It makes no sense. In terms of run time, this patch is fine but in terms of reading the code using min() makes it less readable. regards, dan carpenter
On Mon, Nov 07, 2022 at 04:08:31PM +0300, Dan Carpenter wrote: > On Mon, Nov 07, 2022 at 09:40:00AM +0530, Deepak R Varma wrote: > > Simplify code by using recommended min helper macro for logical > > evaluation and value assignment. This issue is identified by > > coccicheck using the minmax.cocci file. > > > > Signed-off-by: Deepak R Varma <drv@mailo.com> > > --- > > drivers/staging/iio/meter/ade7854-i2c.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c > > index a9a06e8dda51..a6ce7b24cc8f 100644 > > --- a/drivers/staging/iio/meter/ade7854-i2c.c > > +++ b/drivers/staging/iio/meter/ade7854-i2c.c > > @@ -61,7 +61,7 @@ static int ade7854_i2c_write_reg(struct device *dev, > > unlock: > > mutex_unlock(&st->buf_lock); > > > > - return ret < 0 ? ret : 0; > > + return min(ret, 0); > > The original code is better. > > If it's a failure return the error code. If it's not return zero. > > You can only compare apples to apples. min() makes sense if you're > talking about two lengths. But here if ret is negative that's an error > code. If it's positive that's the number of bytes. If the error > code is less than the number of bytes then return that? What??? It > makes no sense. Thanks for your view point. I agree. > > In terms of run time, this patch is fine but in terms of reading the > code using min() makes it less readable. Okay, The proposal does not make much difference, so will leave the original line as is. Thank you, ./drv > > regards, > dan carpenter >
On Mon, 2022-11-07 at 16:08 +0300, Dan Carpenter wrote: > On Mon, Nov 07, 2022 at 09:40:00AM +0530, Deepak R Varma wrote: > > Simplify code by using recommended min helper macro for logical > > evaluation and value assignment. This issue is identified by > > coccicheck using the minmax.cocci file. > > > > Signed-off-by: Deepak R Varma <drv@mailo.com> > > --- > > drivers/staging/iio/meter/ade7854-i2c.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c > > index a9a06e8dda51..a6ce7b24cc8f 100644 > > --- a/drivers/staging/iio/meter/ade7854-i2c.c > > +++ b/drivers/staging/iio/meter/ade7854-i2c.ck > > @@ -61,7 +61,7 @@ static int ade7854_i2c_write_reg(struct device *dev, > > unlock: > > mutex_unlock(&st->buf_lock); > > > > - return ret < 0 ? ret : 0; > > + return min(ret, 0); > > The original code is better. > > If it's a failure return the error code. If it's not return zero. > > You can only compare apples to apples. min() makes sense if you're > talking about two lengths. But here if ret is negative that's an error > code. If it's positive that's the number of bytes. If the error > code is less than the number of bytes then return that? What??? It > makes no sense. > > In terms of run time, this patch is fine but in terms of reading the > code using min() makes it less readable. It's not a runtime question, either should compile to the same object code. It's definitely a readabiity and standardization issue. In this case, IMO it'd be better to use the much more common if (ret < 0) return ret; return 0;
On Mon, Nov 07, 2022 at 07:22:24AM -0800, Joe Perches wrote: > > In terms of run time, this patch is fine but in terms of reading the > > code using min() makes it less readable. > > It's not a runtime question, either should compile to the same object > code. It's definitely a readabiity and standardization issue. > > In this case, IMO it'd be better to use the much more common > > if (ret < 0) > return ret; > > return 0; I also prefer this format. But at the same time, I can't advise Deepak to go around changing existing code where the author like ternaries. regards, dan carpenter
On Mon, Nov 07, 2022 at 06:38:35PM +0300, Dan Carpenter wrote: > On Mon, Nov 07, 2022 at 07:22:24AM -0800, Joe Perches wrote: > > > In terms of run time, this patch is fine but in terms of reading the > > > code using min() makes it less readable. > > > > It's not a runtime question, either should compile to the same object > > code. It's definitely a readabiity and standardization issue. > > > > In this case, IMO it'd be better to use the much more common > > > > if (ret < 0) > > return ret; > > > > return 0; > > I also prefer this format. > > But at the same time, I can't advise Deepak to go around changing > existing code where the author like ternaries. Thank you Joe, Dan. Just to conclude, I will leave the line untouched as it is no big advantage and the current format is more readable. ./drv > > regards, > dan carpenter >
On Mon, Nov 07, 2022 at 09:40:00AM +0530, Deepak R Varma wrote: > Simplify code by using recommended min helper macro for logical > evaluation and value assignment. This issue is identified by > coccicheck using the minmax.cocci file. > > Signed-off-by: Deepak R Varma <drv@mailo.com> > --- > drivers/staging/iio/meter/ade7854-i2c.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c > index a9a06e8dda51..a6ce7b24cc8f 100644 > --- a/drivers/staging/iio/meter/ade7854-i2c.c > +++ b/drivers/staging/iio/meter/ade7854-i2c.c > @@ -61,7 +61,7 @@ static int ade7854_i2c_write_reg(struct device *dev, > unlock: > mutex_unlock(&st->buf_lock); > > - return ret < 0 ? ret : 0; > + return min(ret, 0); As others have said, this isn't ok, and I hate ? : usage, so if you want, spell that out please. thanks, greg k-h
On Tue, Nov 08, 2022 at 04:12:17PM +0100, Greg Kroah-Hartman wrote: > On Mon, Nov 07, 2022 at 09:40:00AM +0530, Deepak R Varma wrote: > > Simplify code by using recommended min helper macro for logical > > evaluation and value assignment. This issue is identified by > > coccicheck using the minmax.cocci file. > > > > Signed-off-by: Deepak R Varma <drv@mailo.com> > > --- > > drivers/staging/iio/meter/ade7854-i2c.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c > > index a9a06e8dda51..a6ce7b24cc8f 100644 > > --- a/drivers/staging/iio/meter/ade7854-i2c.c > > +++ b/drivers/staging/iio/meter/ade7854-i2c.c > > @@ -61,7 +61,7 @@ static int ade7854_i2c_write_reg(struct device *dev, > > unlock: > > mutex_unlock(&st->buf_lock); > > > > - return ret < 0 ? ret : 0; > > + return min(ret, 0); > > As others have said, this isn't ok, and I hate ? : usage, so if you > want, spell that out please. Hello Greg, Just want to make sure I am getting it right: Are you suggesting me to resubmit the patch with revised patch description? Should I consider using the "if" based evaluation rather than using min() macro? Thank you, ./drv > > thanks, > > greg k-h >
On Tue, 8 Nov 2022 21:06:24 +0530 Deepak R Varma <drv@mailo.com> wrote: > On Tue, Nov 08, 2022 at 04:12:17PM +0100, Greg Kroah-Hartman wrote: > > On Mon, Nov 07, 2022 at 09:40:00AM +0530, Deepak R Varma wrote: > > > Simplify code by using recommended min helper macro for logical > > > evaluation and value assignment. This issue is identified by > > > coccicheck using the minmax.cocci file. > > > > > > Signed-off-by: Deepak R Varma <drv@mailo.com> > > > --- > > > drivers/staging/iio/meter/ade7854-i2c.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c > > > index a9a06e8dda51..a6ce7b24cc8f 100644 > > > --- a/drivers/staging/iio/meter/ade7854-i2c.c > > > +++ b/drivers/staging/iio/meter/ade7854-i2c.c > > > @@ -61,7 +61,7 @@ static int ade7854_i2c_write_reg(struct device *dev, > > > unlock: > > > mutex_unlock(&st->buf_lock); > > > > > > - return ret < 0 ? ret : 0; > > > + return min(ret, 0); > > > > As others have said, this isn't ok, and I hate ? : usage, so if you > > want, spell that out please. > > Hello Greg, > Just want to make sure I am getting it right: > Are you suggesting me to resubmit the patch with revised patch description? > > Should I consider using the "if" based evaluation rather than using min() macro? For IIO staging drivers, I'd take a cleanup that moved to if (ret < 0) return ret; return 0; As others have suggested though, not a good idea to do this broadly as it would be a lot of noise. We don't mind noise so much for staging drivers :) Jonathan > > Thank you, > ./drv > > > > > thanks, > > > > greg k-h > > > >
diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c index a9a06e8dda51..a6ce7b24cc8f 100644 --- a/drivers/staging/iio/meter/ade7854-i2c.c +++ b/drivers/staging/iio/meter/ade7854-i2c.c @@ -61,7 +61,7 @@ static int ade7854_i2c_write_reg(struct device *dev, unlock: mutex_unlock(&st->buf_lock); - return ret < 0 ? ret : 0; + return min(ret, 0); } static int ade7854_i2c_read_reg(struct device *dev,
Simplify code by using recommended min helper macro for logical evaluation and value assignment. This issue is identified by coccicheck using the minmax.cocci file. Signed-off-by: Deepak R Varma <drv@mailo.com> --- drivers/staging/iio/meter/ade7854-i2c.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.34.1