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

[Herve Codina <herve.codina@bootlin.com>]

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é