Re: [PATCH v2 2/6] iio: light: Add gain-time-scale helpers

[Matti Vaittinen <mazziesaccount@gmail.com>]

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],
>>>>>> +					&gts->avail_all_scales_table[i * 2],
>>>>>> +					&gts->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! ~~