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

[Herve Codina]

Hi David, Andy

On Tue, 20 Jun 2023 11:45:01 +0000
David Laight <David.Laight@xxxxxxxxxx> 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@xxxxxxxxxxx>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> > ---
> >  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é

-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com