diff mbox series

[v4,07/13] minmax: Introduce {min,max}_array()

Message ID 20230614074904.29085-8-herve.codina@bootlin.com
State New
Headers
Series Add support for IIO devices in ASoC |

Commit Message

Herve Codina June 14, 2023, 7:48 a.m. UTC 
  Introduce min_array() (resp max_array()) in order to get the
minimal (resp maximum) of values present in an array.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 include/linux/minmax.h | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

Comments

Andy Shevchenko June 14, 2023, 9:02 a.m. UTC | #1 
On Wed, Jun 14, 2023 at 10:49 AM Herve Codina <herve.codina@bootlin.com> wrote:
>
> Introduce min_array() (resp max_array()) in order to get the
> minimal (resp maximum) of values present in an array.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
See a remark below.

> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  include/linux/minmax.h | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/include/linux/minmax.h b/include/linux/minmax.h
> index 396df1121bff..2cd0d34ce921 100644
> --- a/include/linux/minmax.h
> +++ b/include/linux/minmax.h
> @@ -133,6 +133,42 @@
>   */
>  #define max_t(type, x, y)      __careful_cmp((type)(x), (type)(y), >)
>
> +/*
> + * 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< ---
> + */
> +#define __minmax_array(op, array, len) ({                      \
> +       typeof(array) __array = (array);                        \
> +       typeof(len) __len = (len);                              \
> +       typeof(__array[0] + 0) __element = __array[--__len];    \

Do we need the ' + 0' part?

> +       while (__len--)                                         \
> +               __element = op(__element, __array[__len]);      \
> +       __element; })
> +
> +/**
> + * min_array - return minimum of values present in an array
> + * @array: array
> + * @len: array length
> + *
> + * Note that @len must not be zero (empty array).
> + */
> +#define min_array(array, len) __minmax_array(min, array, len)
> +
> +/**
> + * max_array - return maximum of values present in an array
> + * @array: array
> + * @len: array length
> + *
> + * Note that @len must not be zero (empty array).
> + */
> +#define max_array(array, len) __minmax_array(max, array, len)
> +
>  /**
>   * clamp_t - return a value clamped to a given range using a given type
>   * @type: the type of variable to use
> --
> 2.40.1
>
Herve Codina June 14, 2023, 9:42 a.m. UTC | #2 
Hi Andy,

On Wed, 14 Jun 2023 12:02:57 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Jun 14, 2023 at 10:49 AM Herve Codina <herve.codina@bootlin.com> wrote:
> >
> > Introduce min_array() (resp max_array()) in order to get the
> > minimal (resp maximum) of values present in an array.  
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> See a remark below.
> 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---
> >  include/linux/minmax.h | 36 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >
> > diff --git a/include/linux/minmax.h b/include/linux/minmax.h
> > index 396df1121bff..2cd0d34ce921 100644
> > --- a/include/linux/minmax.h
> > +++ b/include/linux/minmax.h
> > @@ -133,6 +133,42 @@
> >   */
> >  #define max_t(type, x, y)      __careful_cmp((type)(x), (type)(y), >)
> >
> > +/*
> > + * 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< ---
> > + */
> > +#define __minmax_array(op, array, len) ({                      \
> > +       typeof(array) __array = (array);                        \
> > +       typeof(len) __len = (len);                              \
> > +       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.

Regards,
Hervé

> 
> > +       while (__len--)                                         \
> > +               __element = op(__element, __array[__len]);      \
> > +       __element; })
> > +
> > +/**
> > + * min_array - return minimum of values present in an array
> > + * @array: array
> > + * @len: array length
> > + *
> > + * Note that @len must not be zero (empty array).
> > + */
> > +#define min_array(array, len) __minmax_array(min, array, len)
> > +
> > +/**
> > + * max_array - return maximum of values present in an array
> > + * @array: array
> > + * @len: array length
> > + *
> > + * Note that @len must not be zero (empty array).
> > + */
> > +#define max_array(array, len) __minmax_array(max, array, len)
> > +
> >  /**
> >   * clamp_t - return a value clamped to a given range using a given type
> >   * @type: the type of variable to use
> > --
> > 2.40.1
> >  
> 
>
Andy Shevchenko June 14, 2023, 11:51 a.m. UTC | #3 
On Wed, Jun 14, 2023 at 12:42 PM Herve Codina <herve.codina@bootlin.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 AM Herve Codina <herve.codina@bootlin.com> wrote:

...

> > > +       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.
Herve Codina June 14, 2023, 8:34 p.m. UTC | #4 
Hi Andy,

On Wed, 14 Jun 2023 14:51:43 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Jun 14, 2023 at 12:42 PM Herve Codina <herve.codina@bootlin.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 AM Herve Codina <herve.codina@bootlin.com> wrote:  
> 
> ...
> 
> > > > +       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
 * promotion. For instance, a const s8 __array[0] lead to an int __element due
 * 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.
 */
#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]); \
	__element; })

/**
 * min_array - return minimum of values present in an array
 * @array: array
 * @len: array length
 *
 * Note that @len must not be zero (empty array).
 */
#define min_array(array, len) __minmax_array(min_t, array, len)

/**
 * max_array - return maximum of values present in an array
 * @array: array
 * @len: array length
 *
 * Note that @len must not be zero (empty array).
 */
#define max_array(array, len) __minmax_array(max_t, array, len)

--- 8< ---

Tested ok from user-space on my x86_64 using the following types for *buff
and buff[N]:
- signed/unsigned char
- signed/unsigned short
- signed/unsigned int
- signed/unsigned long
- signed/unsigned long long
- float, double, long double (even if not used in the kernel)

Can you give me your feedback on this last version ?

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 switch
back to the specific min/max computation in IIO inkern.c

Sorry for this back and forth and this last minute detected bug.

Best regards,
Hervé
Andy Shevchenko June 14, 2023, 10:05 p.m. UTC | #5 
On Wed, Jun 14, 2023 at 11:34 PM 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 PM Herve Codina <herve.codina@bootlin.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 AM Herve Codina <herve.codina@bootlin.com> wrote:

...

> > > > > +       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).

>         __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 switch
> 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?
Herve Codina June 15, 2023, 9:35 a.m. UTC | #6 
Hi Andy,
On Thu, 15 Jun 2023 01:05:40 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Jun 14, 2023 at 11:34 PM 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 PM Herve Codina <herve.codina@bootlin.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 AM Herve Codina <herve.codina@bootlin.com> wrote:  
> 
> ...
> 
> > > > > > +       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.

Did the job using _Generic().

This lead to:
--- 8< ---
/*
 * Remove a const qualifier
 * _Generic(foo, type-name: association, ..., default: association) performs a
 * comparison against the foo type (not the qualified type).
 * Do not use the const keyword in the type-name as it will not match the
 * unqualified type of foo.
 */
#define __unconst_type_cases(type)		\
	unsigned type:  (unsigned type)0,	\
	signed type:    (signed type)0


#define __unconst_typeof(x) typeof(			\
	_Generic((x),					\
		char: (char)0,				\
		__unconst_type_cases(char),		\
		__unconst_type_cases(short),		\
		__unconst_type_cases(int),		\
		__unconst_type_cases(long),		\
		__unconst_type_cases(long long),	\
		default: (x)))

/*
 * 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.
 *
 * The array can be an array of const items.
 * typeof() keeps the const qualifier. Use __unconst_typeof() in order to
 * discard the const qualifier for the __element variable.
 */
#define __minmax_array(op, array, len) ({				\
	typeof(&(array)[0]) __array = (array);				\
	typeof(len) __len = (len);					\
	__unconst_typeof(__array[0]) __element = __array[--__len];	\
	while (__len--)							\
		__element = op(__element, __array[__len]);		\
	__element; })

/**
 * min_array - return minimum of values present in an array
 * @array: array
 * @len: array length
 *
 * Note that @len must not be zero (empty array).
 */
#define min_array(array, len) __minmax_array(min, array, len)

/**
 * max_array - return maximum of values present in an array
 * @array: array
 * @len: array length
 *
 * Note that @len must not be zero (empty array).
 */
#define max_array(array, len) __minmax_array(max, array, len)
--- 8< ---

Do you think it looks good ?

For, the KUnit tests, I agree, it would be nice to have something.
I need some more substantial work to implement and run the test in KUnit
and the first task will be learning the KUnit test system. 
I will do that but out of this series.

Thanks for your feedback and pointers,
Hervé
Andy Shevchenko June 15, 2023, 1:51 p.m. UTC | #7 
On Thu, Jun 15, 2023 at 12:35 PM Herve Codina <herve.codina@bootlin.com> wrote:
> On Thu, 15 Jun 2023 01:05:40 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

...

> Did the job using _Generic().

Cool! Keep my tag for that version and thank you for pursuing the
implementation that works for everybody.

> This lead to:
> --- 8< ---
> /*
>  * Remove a const qualifier

...from integer types

>  * _Generic(foo, type-name: association, ..., default: association) performs a
>  * comparison against the foo type (not the qualified type).
>  * Do not use the const keyword in the type-name as it will not match the
>  * unqualified type of foo.
>  */
> #define __unconst_type_cases(type)              \

__unconst_integer_type_cases() ?

>         unsigned type:  (unsigned type)0,       \
>         signed type:    (signed type)0
>
>

Single blank line is enough.

> #define __unconst_typeof(x) typeof(                     \

__unconst_integer_typeof() ?

>         _Generic((x),                                   \
>                 char: (char)0,                          \
>                 __unconst_type_cases(char),             \
>                 __unconst_type_cases(short),            \
>                 __unconst_type_cases(int),              \
>                 __unconst_type_cases(long),             \
>                 __unconst_type_cases(long long),        \
>                 default: (x)))
>
> /*
>  * 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.
>  *
>  * The array can be an array of const items.
>  * typeof() keeps the const qualifier. Use __unconst_typeof() in order to
>  * discard the const qualifier for the __element variable.
>  */
> #define __minmax_array(op, array, len) ({                               \
>         typeof(&(array)[0]) __array = (array);                          \
>         typeof(len) __len = (len);                                      \
>         __unconst_typeof(__array[0]) __element = __array[--__len];      \
>         while (__len--)                                                 \
>                 __element = op(__element, __array[__len]);              \
>         __element; })
>
> /**
>  * min_array - return minimum of values present in an array
>  * @array: array
>  * @len: array length
>  *
>  * Note that @len must not be zero (empty array).
>  */
> #define min_array(array, len) __minmax_array(min, array, len)
>
> /**
>  * max_array - return maximum of values present in an array
>  * @array: array
>  * @len: array length
>  *
>  * Note that @len must not be zero (empty array).
>  */
> #define max_array(array, len) __minmax_array(max, array, len)
> --- 8< ---
>
> Do you think it looks good ?

Yes!

> For, the KUnit tests, I agree, it would be nice to have something.
> I need some more substantial work to implement and run the test in KUnit
> and the first task will be learning the KUnit test system.
> I will do that but out of this series.

Thank you, it's fine with me.
David Laight June 16, 2023, 9:08 a.m. UTC | #8 
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)
Herve Codina June 16, 2023, 11:48 a.m. UTC | #9 
Hi David,

On Fri, 16 Jun 2023 09:08:22 +0000
David Laight <David.Laight@ACULAB.COM> wrote:

...

> 
> 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.

I finally move to use _Generic() in order to "unconstify" and avoid the
integer promotion. With this done, no extra cast is needed and min()/max()
are usable.

The patch is available in the v5 series.
  https://lore.kernel.org/linux-kernel/20230615152631.224529-8-herve.codina@bootlin.com/

Do you think the code present in the v5 series should be changed ?
If so, can you give me your feedback on the v5 series ?

Thanks for your review,
Hervé
David Laight June 16, 2023, 12:42 p.m. UTC | #10 
From: Herve Codina <herve.codina@bootlin.com>
> Sent: 16 June 2023 12:49
> 
> Hi David,
> 
> On Fri, 16 Jun 2023 09:08:22 +0000
> David Laight <David.Laight@ACULAB.COM> wrote:
> 
> ...
> 
> >
> > 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.
> 
> I finally move to use _Generic() in order to "unconstify" and avoid the
> integer promotion. With this done, no extra cast is needed and min()/max()
> are usable.
> 
> The patch is available in the v5 series.
>   https://lore.kernel.org/linux-kernel/20230615152631.224529-8-herve.codina@bootlin.com/
> 
> Do you think the code present in the v5 series should be changed ?
> If so, can you give me your feedback on the v5 series ?

It seems horribly over-complicated just to get around the perverse
over-strong type checking that min/max do just to avoid sign
extension issues.

Maybe I ought to try getting a patch accepted that just checks
  is_signed_type(typeof(x)) == is_signed_type(typeof(y))
instead of
  typeof(x) == typeof(y)
Then worry about the valid signed v unsigned cases.

Indeed, since the array index can be assumed not to have side
effects you could use __cmp(x, y, op) directly.

No one has pointed out that __element should be __bound.

	David

	

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
diff mbox series

Patch

diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index 396df1121bff..2cd0d34ce921 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -133,6 +133,42 @@ 
  */
 #define max_t(type, x, y)	__careful_cmp((type)(x), (type)(y), >)
 
+/*
+ * 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< ---
+ */
+#define __minmax_array(op, array, len) ({			\
+	typeof(array) __array = (array);			\
+	typeof(len) __len = (len);				\
+	typeof(__array[0] + 0) __element = __array[--__len];	\
+	while (__len--)						\
+		__element = op(__element, __array[__len]);	\
+	__element; })
+
+/**
+ * min_array - return minimum of values present in an array
+ * @array: array
+ * @len: array length
+ *
+ * Note that @len must not be zero (empty array).
+ */
+#define min_array(array, len) __minmax_array(min, array, len)
+
+/**
+ * max_array - return maximum of values present in an array
+ * @array: array
+ * @len: array length
+ *
+ * Note that @len must not be zero (empty array).
+ */
+#define max_array(array, len) __minmax_array(max, array, len)
+
 /**
  * clamp_t - return a value clamped to a given range using a given type
  * @type: the type of variable to use