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

  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>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 include/linux/minmax.h | 64 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)
  

On Thu, Jun 15, 2023 at 6:26 PM 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.

Only in the case if you need to send a new version (otherwise a follow
up will be okay)...

...

> + * The first typeof(&(array)[0]) is needed in order to support arrays of both
> + * 'int *buff' and 'int buf[N]' types.

int buff[N] ?

> + * The array can be an array of const items.
> + * typeof() keeps the const qualifier. Use __unconst_typeof() in order to

+ _integer

> + * discard the const qualifier for the __element variable.
  

From: Herve Codina
> Sent: 15 June 2023 16:26
> 
> 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>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
>  include/linux/minmax.h | 64 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/include/linux/minmax.h b/include/linux/minmax.h
> index 396df1121bff..1672985b02a3 100644
> --- a/include/linux/minmax.h
> +++ b/include/linux/minmax.h
> @@ -133,6 +133,70 @@
>   */
>  #define max_t(type, x, y)	__careful_cmp((type)(x), (type)(y), >)
> 
> +/*
> + * 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_integer_type_cases(type)	\
> +	unsigned type:  (unsigned type)0,	\
> +	signed type:    (signed type)0
> +
> +#define __unconst_integer_typeof(x) typeof(			\
> +	_Generic((x),						\
> +		char: (char)0,					\
> +		__unconst_integer_type_cases(char),		\
> +		__unconst_integer_type_cases(short),		\
> +		__unconst_integer_type_cases(int),		\
> +		__unconst_integer_type_cases(long),		\
> +		__unconst_integer_type_cases(long long),	\
> +		default: (x)))

Those are probably more generally useful and belong elsewhere.

> +
> +/*
> + * 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< ---

Is that needed in the .h file?

> + *
> + * 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_integer_typeof(__array[0]) __element = __array[--__len]; \

s/__element/__bound/

> +	while (__len--)							\
> +		__element = op(__element, __array[__len]);		\
> +	__element; })

I'm not all sure that all the shenanigans required to use min()
is really needed here.

It would also be generally better to process the array forwards.
So something like:
	typeof (&array[0]) __ptr = array, __limit = array + len;
	typeof (array[0] + 0) __element, __bound = *__ptr++;
	while (ptr < __limit) {
		__element = *__ptr++;
		if (__element > __bound)
			__bound = __element;
	}
	(typeof (array[0]))__bound; })
seems fine to me.
The final cast is there to convert 'int' back to un/signed char|short.
Not really needed and might generate worse code.

But if you insist on using min/max ignore this bit.

	David

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

Hi David, Andy

On Tue, 20 Jun 2023 11:45:01 +0000
David Laight <David.Laight@ACULAB.COM> wrote:

> From: Herve Codina
> > Sent: 15 June 2023 16:26
> > 
> > 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>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > ---
> >  include/linux/minmax.h | 64 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> > 
> > diff --git a/include/linux/minmax.h b/include/linux/minmax.h
> > index 396df1121bff..1672985b02a3 100644
> > --- a/include/linux/minmax.h
> > +++ b/include/linux/minmax.h
> > @@ -133,6 +133,70 @@
> >   */
> >  #define max_t(type, x, y)	__careful_cmp((type)(x), (type)(y), >)
> > 
> > +/*
> > + * 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_integer_type_cases(type)	\
> > +	unsigned type:  (unsigned type)0,	\
> > +	signed type:    (signed type)0
> > +
> > +#define __unconst_integer_typeof(x) typeof(			\
> > +	_Generic((x),						\
> > +		char: (char)0,					\
> > +		__unconst_integer_type_cases(char),		\
> > +		__unconst_integer_type_cases(short),		\
> > +		__unconst_integer_type_cases(int),		\
> > +		__unconst_integer_type_cases(long),		\
> > +		__unconst_integer_type_cases(long long),	\
> > +		default: (x)))  
> 
> Those are probably more generally useful and belong elsewhere.

Yes but it is only used here.
It can be move somewhere, in a common place, when necessary.

> 
> > +
> > +/*
> > + * 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< ---  
> 
> Is that needed in the .h file?
> 
> > + *
> > + * 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_integer_typeof(__array[0]) __element = __array[--__len]; \  
> 
> s/__element/__bound/
> 
> > +	while (__len--)							\
> > +		__element = op(__element, __array[__len]);		\
> > +	__element; })  
> 
> I'm not all sure that all the shenanigans required to use min()
> is really needed here.
> 
> It would also be generally better to process the array forwards.
> So something like:
> 	typeof (&array[0]) __ptr = array, __limit = array + len;
> 	typeof (array[0] + 0) __element, __bound = *__ptr++;
> 	while (ptr < __limit) {
> 		__element = *__ptr++;
> 		if (__element > __bound)
> 			__bound = __element;
> 	}
> 	(typeof (array[0]))__bound; })
> seems fine to me.
> The final cast is there to convert 'int' back to un/signed char|short.
> Not really needed and might generate worse code.
> 
> But if you insist on using min/max ignore this bit.

I didn't plan to change the {min,max}_array() macros in this series as you
suggest.

Maybe min()/max() is too strict but it's a way to be sure about the type
used. Also the current version doesn't need any extra cast to get rid of
the integer promotion as the integer promotion doesn't occur.

Is it ok for you if we keep as it ?

Thanks for your feedback,
Hervé
  

On Thu, Jun 22, 2023 at 02:32:33PM +0200, Herve Codina wrote:

> Is it ok for you if we keep as it ?

I think any issues here can be addressed incrementally rather than
holding up the rest of the series.
  

On Thu, Jun 22, 2023 at 3:32 PM Herve Codina <herve.codina@bootlin.com> wrote:
> On Tue, 20 Jun 2023 11:45:01 +0000
> David Laight <David.Laight@ACULAB.COM> wrote:
> > From: Herve Codina
> > > Sent: 15 June 2023 16:26

> I didn't plan to change the {min,max}_array() macros in this series as you
> suggest.
>
> Maybe min()/max() is too strict but it's a way to be sure about the type
> used. Also the current version doesn't need any extra cast to get rid of
> the integer promotion as the integer promotion doesn't occur.
>
> Is it ok for you if we keep as it ?

I think the current verison is good enough, and as Mark said we may
update incrementally if ever needed.