Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230614074904.29085-1-herve.codina@bootlin.com>
 <20230614074904.29085-8-herve.codina@bootlin.com> <CAHp75Vcur=H_2mBm5Ztuvd7Jnvmr6+tvCbEkFtmaVLsEjXr8NQ@mail.gmail.com>
 <20230614114214.1371485e@bootlin.com> <CAHp75VcmW2StPqb_LtKFyNyJ2+jz3c19zNRDiSuGs06Bseq04w@mail.gmail.com>
 <20230614223418.0d7e355d@bootlin.com>
In-Reply-To: <20230614223418.0d7e355d@bootlin.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Thu, 15 Jun 2023 01:05:40 +0300
Message-ID: <CAHp75VfFyDzr4qHNssXZ8RLy0gxMWdjBgac4JLd7grRLEG-vyw@mail.gmail.com>
Subject: Re: [PATCH v4 07/13] minmax: Introduce {min,max}_array()
To:     Herve Codina <herve.codina@bootlin.com>
Cc:     Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>,
        Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>,
        alsa-devel@alsa-project.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
        Christophe Leroy <christophe.leroy@csgroup.eu>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Jun 14, 2023 at 11:34=E2=80=AFPM Herve Codina <herve.codina@bootlin=
.com> wrote:
> On Wed, 14 Jun 2023 14:51:43 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Wed, Jun 14, 2023 at 12:42=E2=80=AFPM Herve Codina <herve.codina@boo=
tlin.com> wrote:
> > > On Wed, 14 Jun 2023 12:02:57 +0300
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > On Wed, Jun 14, 2023 at 10:49=E2=80=AFAM Herve Codina <herve.codina=
@bootlin.com> wrote:

...

> > > > > +       typeof(__array[0] + 0) __element =3D __array[--__len];   =
 \
> > > >
> > > > Do we need the ' + 0' part?
> > >
> > > Yes.
> > >
> > > __array can be an array of const items and it is legitimate to get th=
e
> > > minimum value from const items.
> > >
> > > typeof(__array[0]) keeps the const qualifier but we need to assign __=
element
> > > in the loop.
> > > One way to drop the const qualifier is to get the type from a rvalue =
computed
> > > from __array[0]. This rvalue has to have the exact same type with onl=
y the const
> > > dropped.
> > > '__array[0] + 0' was a perfect canditate.
> >
> > Seems like this also deserves a comment. But if the series is accepted
> > as is, it may be done as a follow up.
> >
>
> Finally not so simple ...
> I did some deeper tests and the macros need to be fixed.
>
> I hope this one (with comments added) is correct:
> --- 8 ---
> /*
>  * Do not check the array parameter using __must_be_array().
>  * In the following legit use-case where the "array" passed is a simple p=
ointer,
>  * __must_be_array() will return a failure.
>  * --- 8< ---
>  * int *buff
>  * ...
>  * min =3D min_array(buff, nb_items);
>  * --- 8< ---
>  *
>  * The first typeof(&(array)[0]) is needed in order to support arrays of =
both
>  * 'int *buff' and 'int buf[N]' types.
>  *
>  * typeof(__array[0] + 0) used for __element is needed as the array can b=
e an
>  * array of const items.
>  * In order to discard the const qualifier use an arithmetic operation (r=
value).


>  * This arithmetic operation discard the const but also can lead to an in=
teger

discards

>  * promotion. For instance, a const s8 __array[0] lead to an int __elemen=
t due

leads

>  * to the promotion.
>  * In this case, simple min() or max() operation fails (type mismatch).
>  * Use min_t() or max_t() (op_t parameter) enforcing the type in order to=
 avoid
>  * the min() or max() failure.

This part perhaps can be avoided. See below.

>  */
> #define __minmax_array(op_t, array, len) ({                     \
>         typeof(&(array)[0]) __array =3D (array);                  \
>         typeof(len) __len =3D (len);                              \
>         typeof(__array[0] + 0) __element =3D __array[--__len];    \
>         while (__len--)                                         \
>                 __element =3D op_t(typeof(__array[0]), __element, __array=
[__len]); \

But can't we instead have typeof(+(array[0])) in the definition of __elemen=
t?
There are also other possible solutions: a) _Generic() with listed
const types to move them to non-const, and b) __auto_type (which is
supported by GCC 4.9 and clang, but not in the C11 standard).

>         __element; })

...

> Can you give me your feedback on this last version ?

Sure!

> If you are ok, it will be present in the next iteration of the series.
> Otherwise, as a last ressort, I will drop the {min,max}_array() and switc=
h
> back to the specific min/max computation in IIO inkern.c
>
> Sorry for this back and forth and this last minute detected bug.

It's good that we have some tests (perhaps you can add something to
unit test these)? Perhaps move your code to lib/test_minmax.c as KUnit
module?

--=20
With Best Regards,
Andy Shevchenko