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

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Matti Vaittinen <mazziesaccount@gmail.com>
Cc: "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>,
	Jonathan Cameron <jic23@kernel.org>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Paul Gazzillo <paul@pgazz.com>,
	Dmitry Osipenko <dmitry.osipenko@collabora.com>,
	Shreeya Patel <shreeya.patel@collabora.com>,
	Zhigang Shi <Zhigang.Shi@liteon.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>
Subject: Re: [PATCH v2 2/6] iio: light: Add gain-time-scale helpers
Date: Tue, 14 Mar 2023 13:36:54 +0200	[thread overview]
Message-ID: <ZBBcViaVsyQFdpkh@smile.fi.intel.com> (raw)
In-Reply-To: <ZBBa+e9VXj/eyT4J@smile.fi.intel.com>

On Tue, Mar 14, 2023 at 01:31:06PM +0200, 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:

...

> > > > > > > > +			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.

JFYI:
$ git grep -n 'diff.* == 0[^0-9]' -- drivers/ | wc -l
45

(It happens to have same variable name, but you can imagine that there are
 much more cases with different variable names in use)

-- 
With Best Regards,
Andy Shevchenko