lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAHp75Vcxwke9Uw5+7wVz9vuMT9xDASCekMQqa4mdsznjSvpEoQ@mail.gmail.com>
Date:   Wed, 14 Feb 2018 13:32:26 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Bartosz Golaszewski <brgl@...ev.pl>
Cc:     Philipp Zabel <p.zabel@...gutronix.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Sekhar Nori <nsekhar@...com>,
        Kevin Hilman <khilman@...libre.com>,
        David Lechner <david@...hnology.com>
Subject: Re: [PATCH v2] reset: add support for non-DT systems

On Wed, Feb 14, 2018 at 10:59 AM, Bartosz Golaszewski <brgl@...ev.pl> wrote:
> 2018-02-13 20:17 GMT+01:00 Andy Shevchenko <andy.shevchenko@...il.com>:
>> On Tue, Feb 13, 2018 at 8:39 PM, Bartosz Golaszewski <brgl@...ev.pl> wrote:

>>> +               for (index = 0; lookup->dev; index++, lookup++) {
>>> +                       if (strcmp(dev_id, lookup->dev))
>>> +                               continue;
>>> +
>>> +                       if ((!id && !lookup->id) ||
>>> +                           (id && lookup->id && !strcmp(id, lookup->id))) {
>>> +                               rstc = __reset_control_get_internal(rcdev,
>>> +                                                               index, shared);
>>> +                               break;
>>> +                       }
>>
>> Wouldn't be slightly more readable
>>
>>       if (id && lookup->id) {
>>         if (strcmp(id, lookup->id))
>>           continue;
>>       } else if (id || lookup->id) {
>>         continue;
>>       }
>>
>>       rstc = __reset_control_get_internal(rcdev, index, shared);
>>       break;
>>
>
> You'd get less indentations, yes but I wanted to emphasize the
> condition on which we want to stop in this line.

It doesn't matter (indentation) here, esp. taking into consideration
that you already have another condition by which you continue the
loop.
My proposal adds consistency and from my point of view makes easier to parse.

You check all false conditions in a row, end up with a true one.

>>> +       if (!rstc)
>>> +               return optional ? NULL : ERR_PTR(-ENOENT);
>>
>> Isn't simpler
>>
>> if (!optional && !rstc) // or other way around, depending which might
>> be more offten
>>  return ERR_PTR(...);
>>
>
> IMO it's just a matter of taste.

I think in my proposal it's more straight forward. Easier to parse by reader.

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ