Message-ID: <a5b2c698-0ada-fb3e-01f4-f50c42811163@fi.rohmeurope.com>
Date:   Wed, 15 Mar 2023 07:20:50 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Matti Vaittinen <mazziesaccount@...il.com>
CC:     Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Paul Gazzillo <paul@...zz.com>,
        Dmitry Osipenko <dmitry.osipenko@...labora.com>,
        Shreeya Patel <shreeya.patel@...labora.com>,
        Zhigang Shi <Zhigang.Shi@...eon.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>
Subject: Re: [PATCH v2 2/6] iio: light: Add gain-time-scale helpers

On 3/14/23 13:31, Andy Shevchenko wrote:
> On Tue, Mar 14, 2023 at 12:28:43PM +0200, Matti Vaittinen wrote:
>> 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.
> 
> So, we may start to have a "fundamental disagreements between Matti and Andy on
> the code style in the Linux kernel" document that we won't clash on this again.
> At least the amount of these disagreements seems not decreasing in time.
> 

:)

> ...
> 
>>>>>>>> +			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
> 
> Pleading to the quantity and not quality is not an argument, right?

Eh. I think it has been you who has quite often used that as an 
argument, right?

>> other type of checks. To me the argument about people easily missing the !
>> in if () just do not sound reasonable.
> 
> You may theoretically discuss this, I'm telling from my review background
> and real cases.

I think we all have been reading the code for quite a few years. Thus, 
also my statements are done based on real cases, or lack of them 
thereof. Hence a "Shut up, I have the reviewing experience" is not such 
a strong argument in my books ;)

>>> 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.
> 
> Don't you use your common sense?

I try. I believe this is why I so often clash with people who are just 
more or less blindly following their preferred idioms.

>> 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.
> 
> Testing with !diff sounds like it's a boolean kind and makes a false
> impression that all other values are almost the same meaning which is
> not the case. Am I right? That's why diff == 0 shows the exact intention
> here "I would like to check if diff is 0 because this is *special case*".

To me the diff == 0 would definitely read "difference of gains is zero". 
And this is NOT what this check is here for. And no, using !var is _not_ 
a sign of a boolean for me. It might be if this pattern was not used 
throughout the kernel for checking if something is _not set_, 
successful, NULL and yes, often also to see it something is non zero.

> Making !diff creates less visibility on this.
> 
> Result: Fundamental disagreement between us.

We agree on that :)

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Powered by blists - more mailing lists