On Fri, Jul 21, 2023 at 02:28:36PM +0300, Andy Shevchenko wrote:
> On Fri, Jul 21, 2023 at 12:53:53PM +0200, Nuno Sá wrote:
> > On Fri, 2023-07-21 at 13:14 +0300, Andy Shevchenko wrote:
> > > On Fri, Jul 21, 2023 at 09:59:37AM +0200, Nuno Sá wrote:
> > > > On Thu, 2023-07-20 at 23:53 +0300, Andy Shevchenko wrote:
...
> > > > > + struct attribute **attrs, **attr, *clk = NULL;
> > > > > struct iio_dev_attr *p;
> > > > > - struct attribute **attr, *clk = NULL;
> > > > >
> > > > > /* First count elements in any existing group */
> > > > > - if (indio_dev->info->attrs) {
> > > > > - attr = indio_dev->info->attrs->attrs;
> > > > > - while (*attr++ != NULL)
> > > > > + attrs = indio_dev->info->attrs ? indio_dev->info->attrs->attrs :
> > > > > NULL;
> > > > > + if (attrs) {
> > > > > + for (attr = attrs; *attr; attr++)
> > > > > attrcount_orig++;
> > >
> > > > not really related with the change... maybe just mention it in the commit?
> > >
> > > Hmm... It's related to make krealloc_array() to work as expected.
> > >
> >
> > Hmm, I think it's arguable :). while() -> for() it's not really needed unless
> > I'm missing something. You could even initialize 'attrs' to NULL at declaration
> > and keep the above diff minimum.
>
> I'm not a fan of the assignments in the declarations when it potentially can be
> disrupted by a chunk of code and reading the code itself may be harder due to
> an interruption for checking the initial value. Hence, having
>
> + attr = attrs;
> while (... != NULL)
>
> seems enough to be replaced with one liner for-loop.
Note that attrs is reused later, so the above assignment makes it cleaner that
some value is assigned to it. With the original code it's not so obvious.
> > That said, I actually prefer this style (even though some people don't like much
> > the ternary operator).
>
> Thanks!
>
> > > > > }
--
With Best Regards,
Andy Shevchenko
