Message-ID: <ZBBa+e9VXj/eyT4J@smile.fi.intel.com>
Date:   Tue, 14 Mar 2023 13:31:05 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Matti Vaittinen <mazziesaccount@...il.com>
Cc:     "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>,
        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 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?

> 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 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?

> 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*".

Making !diff creates less visibility on this.

Result: Fundamental disagreement between us.

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists