lkml.org 
[lkml]   [2023]   [Mar]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 2/6] iio: light: Add gain-time-scale helpers
    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


    \
     
     \ /
      Last update: 2023-03-27 00:59    [W:2.781 / U:0.716 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site