RE: [PATCH next v4 0/5] minmax: Relax type checks in min() and max().

David Laight posted 5 patches 1 year, 11 months ago
Only 0 patches received!
There is a newer version of this series
RE: [PATCH next v4 0/5] minmax: Relax type checks in min() and max().
Posted by David Laight 1 year, 11 months ago
From: Dan Carpenter
> Sent: 12 January 2024 09:13
> 
> I've often wondered why so many people use min_t(int, size, limit) when
> they really do not want negative sizes...  Is there a performance reason?
> git grep 'min_t(int,' says there are 872 instances of this.  Probably
> some do want negatives but it's a quite small percent.

But you really don't a negative 'size' converted to a large
unsigned value above the limit - that would be worse.
All the type checking is there to stop that happening.

Even with my changes min(int_var, sizeof()) is a compile error.
To do otherwise would really requite the sizeof() to be converted
to int - leaving the other type alone.
Easiest done by using 'int' instead of 'typeof(y)' for the
local variable inside cmp_once().

However Linus didn't like that change so I removed it from the
version that got committed.
It would also bloat the expansion even more.
That could be reduced by expecting min(var, const) not min(const, var)
and only doing all the constant checks on the second argument.

FWIW min_t() should really skip all the type checks.
Once both value have been cast the 'same type' test will pass.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Re: [PATCH next v4 0/5] minmax: Relax type checks in min() and max().
Posted by Dan Carpenter 1 year, 11 months ago
On Fri, Jan 12, 2024 at 12:16:00PM +0000, David Laight wrote:
> From: Dan Carpenter
> > Sent: 12 January 2024 09:13
> > 
> > I've often wondered why so many people use min_t(int, size, limit) when
> > they really do not want negative sizes...  Is there a performance reason?
> > git grep 'min_t(int,' says there are 872 instances of this.  Probably
> > some do want negatives but it's a quite small percent.
> 
> But you really don't a negative 'size' converted to a large
> unsigned value above the limit - that would be worse.
> All the type checking is there to stop that happening.
> 

I understand your changes, it seems like a really nice API.  I was just
asking about min_t(int, in old code.  Just to take the first example
from my git grep:

arch/arm/mach-orion5x/ts78xx-setup.c
   160                  sz = min_t(int, 4 - off, len);
   161                  writesb(io_base, buf, sz);

If len is negative then we write negative bytes to writesb().  What
was the thinking here?

> Even with my changes min(int_var, sizeof()) is a compile error.
> To do otherwise would really requite the sizeof() to be converted
> to int - leaving the other type alone.
> Easiest done by using 'int' instead of 'typeof(y)' for the
> local variable inside cmp_once().

I think I would maybe like a mins() which returns signed values, and
then we would convert all the min() usages to either minu() or mins()
and delete min().

regards,
dan carpenter