From: David Laight <David.Laight@ACULAB.COM>
To: 'Herve Codina' <herve.codina@bootlin.com>,
Andy Shevchenko <andy.shevchenko@gmail.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" <alsa-devel@alsa-project.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
Christophe Leroy <christophe.leroy@csgroup.eu>,
Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Subject: RE: [PATCH v4 07/13] minmax: Introduce {min,max}_array()
Date: Fri, 16 Jun 2023 09:08:22 +0000 [thread overview]
Message-ID: <6c7fe34f7b65421ab618d33ba907ae09@AcuMS.aculab.com> (raw)
In-Reply-To: <20230615113512.07967677@bootlin.com>
From: Herve Codina
> Sent: 15 June 2023 10:35
> > ...
> >
> > > > > > > + typeof(__array[0] + 0) __element = __array[--__len]; \
> > > > > >
> > > > > > Do we need the ' + 0' part?
> > > > >
> > > > > Yes.
> > > > >
> > > > > __array can be an array of const items and it is legitimate to get the
> > > > > 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 only 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 pointer,
> > > * __must_be_array() will return a failure.
> > > * --- 8< ---
> > > * int *buff
> > > * ...
> > > * min = 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 be an
> > > * array of const items.
> > > * In order to discard the const qualifier use an arithmetic operation (rvalue).
> >
> >
> > > * This arithmetic operation discard the const but also can lead to an integer
> >
> > discards
> >
> > > * promotion. For instance, a const s8 __array[0] lead to an int __element 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 = (array); \
> > > typeof(len) __len = (len); \
> > > typeof(__array[0] + 0) __element = __array[--__len]; \
> > > while (__len--) \
> > > __element = op_t(typeof(__array[0]), __element, __array[__len]); \
> >
> > But can't we instead have typeof(+(array[0])) in the definition of __element?
> > 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).
>
> typeof(+(array[0])) keeps the promotion.
>
> __auto_type works with my gcc-12 but not with a gcc-5.5. Depending on the
> compiler version, it discards or keeps the const qualifier. For this reason
> I would prefer to not use it.
Just define two variables typeof(__array[0] + 0) one for an element
and one for the limit.
The just test (eg):
if (limit > item) limit = item;
finally cast the limit back to the original type.
The promotions of char/short to signed int won't matter.
There is no need for all the type-checking in min/max.
Indeed, if min_t(type, a, b) is in anyway sane it should
expand to:
type _a = a, _b = b;
_a < _b ? _a : _b
without any of the checks that min() does.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
next prev parent reply other threads:[~2023-06-16 9:10 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-14 7:48 [PATCH v4 00/13] Add support for IIO devices in ASoC Herve Codina
2023-06-14 7:48 ` [PATCH v4 01/13] ASoC: dt-bindings: Add audio-iio-aux Herve Codina
2023-06-14 7:48 ` [PATCH v4 02/13] ASoC: dt-bindings: simple-card: Add additional-devs subnode Herve Codina
2023-06-14 7:48 ` [PATCH v4 03/13] iio: inkern: Check error explicitly in iio_channel_read_max() Herve Codina
2023-06-14 7:48 ` [PATCH v4 04/13] iio: consumer.h: Fix raw values documentation notes Herve Codina
2023-06-14 7:48 ` [PATCH v4 05/13] iio: inkern: Remove the 'unused' variable usage in iio_channel_read_max() Herve Codina
2023-06-14 7:48 ` [PATCH v4 06/13] iio: inkern: Fix headers inclusion order Herve Codina
2023-06-14 8:58 ` Andy Shevchenko
2023-06-14 7:48 ` [PATCH v4 07/13] minmax: Introduce {min,max}_array() Herve Codina
2023-06-14 9:02 ` Andy Shevchenko
2023-06-14 9:42 ` Herve Codina
2023-06-14 11:51 ` Andy Shevchenko
2023-06-14 20:34 ` Herve Codina
2023-06-14 22:05 ` Andy Shevchenko
2023-06-15 9:35 ` Herve Codina
2023-06-15 13:51 ` Andy Shevchenko
2023-06-16 9:08 ` David Laight [this message]
2023-06-16 11:48 ` Herve Codina
2023-06-16 12:42 ` David Laight
2023-06-14 7:48 ` [PATCH v4 08/13] iio: inkern: Use max_array() to get the maximum value from an array Herve Codina
2023-06-14 9:03 ` Andy Shevchenko
2023-06-14 7:49 ` [PATCH v4 09/13] iio: inkern: Replace a FIXME comment by a TODO one Herve Codina
2023-06-14 7:49 ` [PATCH v4 10/13] iio: inkern: Add a helper to query an available minimum raw value Herve Codina
2023-06-14 7:49 ` [PATCH v4 11/13] ASoC: soc-dapm.h: Convert macros to return a compound literal Herve Codina
2023-06-14 7:49 ` [PATCH v4 12/13] ASoC: codecs: Add support for the generic IIO auxiliary devices Herve Codina
2023-06-14 7:49 ` [PATCH v4 13/13] ASoC: simple-card: Handle additional devices Herve Codina
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6c7fe34f7b65421ab618d33ba907ae09@AcuMS.aculab.com \
--to=david.laight@aculab.com \
--cc=alsa-devel@alsa-project.org \
--cc=andy.shevchenko@gmail.com \
--cc=broonie@kernel.org \
--cc=christophe.leroy@csgroup.eu \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=herve.codina@bootlin.com \
--cc=jic23@kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=kuninori.morimoto.gx@renesas.com \
--cc=lars@metafoo.de \
--cc=lgirdwood@gmail.com \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=perex@perex.cz \
--cc=robh+dt@kernel.org \
--cc=thomas.petazzoni@bootlin.com \
--cc=tiwai@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).