diff mbox series

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

Message ID 20230615152631.224529-8-herve.codina@bootlin.com
State Superseded
Headers show
Series Add support for IIO devices in ASoC | expand

Commit Message

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

Comments

Herve Codina June 22, 2023, 12:32 p.m. UTC | #1
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é
Mark Brown June 22, 2023, 12:37 p.m. UTC | #2
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.
Andy Shevchenko June 22, 2023, 2:08 p.m. UTC | #3
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.
diff mbox series

Patch

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)))
+
+/*
+ * 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_integer_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)
+
 /**
  * clamp_t - return a value clamped to a given range using a given type
  * @type: the type of variable to use