RE: [PATCH v2 1/8] minmax: Put all the clamp() definitions together

[David Laight]

From: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxx>
> Sent: 28 July 2024 21:32
> 
> On Sun, 28 Jul 2024 at 13:23, David Laight <David.Laight@xxxxxxxxxx> wrote:
> >
> > MIN() (and probably your MIN_T()) ought to have a check for
> > being a constant in order to stop misuse.
> 
> No, we have a number of "runtime constants" that are basically
> "constants" set up at boot-time for the architecture,as pointed out by
> the powerpc people in private:
> 
> Ie, we have arch/powerpc/include/asm/page.h:
> 
>    #define HPAGE_SHIFT hpage_shift
> 
> and then
> 
>   #define HUGETLB_PAGE_ORDER      (HPAGE_SHIFT - PAGE_SHIFT)
> 
> and then
> 
>    #define pageblock_order         MIN_T(unsigned int,
> HUGETLB_PAGE_ORDER, MAX_PAGE_ORDER)
> 
> and we really *REALLY* don't want to force the complicated "min_t()"
> (or, worse yet, "min()") functions here just because there's actually
> a variable involved.
> 
> That variable gets initialized early in
> hugetlbpage_init_defaultsize(), so it's *effectively* a constant, but
> not as far as the compiler is concerned.

Ok, but those can't be used as array sizes or constants.
So the temporaries don't matter.
Don't they just work with min() - if not where is the signednes mismatch?

Perhaps we want:
	min() - temporaries, signedness check.
	min_t() - temporaries of given type, maybe check size not reduced?
	MIN() - no temporaries, no signedness check, only valid for constants.
	_min() - temporaries, no signedness check.
	_MIN() - no temporaries, no signedness check, variables allowed.

I'm not sure where your MIN_T() fits in the above.

Personally I think min_t() was a mistake.
Only one input can need a cast and an explicit cast would be safer.

	David

> 
>            Linus

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