Re: [PATCH v2 2/6] iio: light: Add gain-time-scale helpers
From: Matti Vaittinen
Date: Tue Mar 14 2023 - 06:31:05 EST
On 3/13/23 16:39, Andy Shevchenko wrote:
On Mon, Mar 13, 2023 at 01:31:42PM +0200, Matti Vaittinen wrote:
On 3/6/23 13:13, Andy Shevchenko wrote:
On Fri, Mar 03, 2023 at 07:54:22AM +0000, Vaittinen, Matti wrote:
On 3/2/23 17:05, Andy Shevchenko wrote:
On Thu, Mar 02, 2023 at 12:57:54PM +0200, Matti Vaittinen wrote:
...
+ for (i = 0; !ret && i < gts->num_avail_all_scales; i++)
Much easier to read if you move this...
+ ret = iio_gts_total_gain_to_scale(gts, all_gains[i],
+ >s->avail_all_scales_table[i * 2],
+ >s->avail_all_scales_table[i * 2 + 1]);
...here as
if (ret)
break;
I think the !ret in loop condition is obvious. Adding break and brackets
would not improve this.
It moves it to the regular pattern. Yours is not so distributed in the kernel.
I believe we can find examples of both patterns in kernel. I don't think the
"many people use different pattern" is a great reason to add break +
brackets which (in my eyes) give no additional value to code I am planning
to keep reading also in the future...
The problem is that your pattern is not so standard (distributed) and hence
less maintainable.
I am sorry but I can't really agree with you on this one. For me adding
the break and brackets would just complicate the flow and thus decrease
the maintainability.
...
+ if (!diff) {
Why not positive conditional?
Because !diff is a special condition and we check explicitly for it.
And how my suggestion makes it different?
In example you gave we would be checking if the value is anything else but
the specific value we are checking for. It is counter intuitive.
(Note, it's easy to miss the ! in the conditionals, that's why positive ones
are preferable.)
Thank you for explaining me the rationale behind the "positive checks". I
didn't know missing '!' was seen as a thing.
I still don't think being afraid of missing '!' is a good reason to switch
to counter intuitive checks. A check "if (!foo)" is a pattern in-kernel if
anything and in my opinion people really should be aware of it.
(I would much more say that having a constant value on left side of a
"equality" check is beneficial as people do really occasionally miss one '='
when meaning '=='. Still, this is not strong enough reason to make
counter-intuitive checks. In my books 'avoiding negative checks' is much
less of a reason as people (in my experience) do not really miss the '!'.)
It's not a problem when it's a common pattern (like you mentioned
if (!foo) return -ENOMEM; or alike), but in your case it's not.
I think we can find plenty of cases where the if (!foo) is used also for
other type of checks. To me the argument about people easily missing the
! in if () just do not sound reasonable.
I would rather see if (diff == 0) which definitely shows the intention
and I wouldn't tell a word against it.
I think this depends much of the corner of the kernel you have been
working with. As far as I remember, in some parts the kernel the check
(foo == 0) was actually discouraged, and check (!foo) was preferred.
Personally I like !foo much more - but I can tolerate the (foo == 0) in
cases where the purpose is to really see if some measure equals to zero.
Other uses where I definitely don't want to use "== 0" are for example
checking if a flag is clear, pointer is NULL or "magic value" is zero.
In this case we are checking for a magic value. Having this check
written as: (diff == 0), would actually falsely suggest me we are
checking for the difference of gains being zero. That would really be a
clever obfuscation and I am certain the code readers would fall on that
trap quite easily.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~