| Message ID | c6924533f157497b836bff24073934a6@AcuMS.aculab.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | minmax: Optimise to reduce .i line length | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch, async |
On Sun, 25 Feb 2024 at 08:53, David Laight <David.Laight@aculab.com> wrote: > > The expansions of min() and max() contain statement expressions so are > not valid for static intialisers. > min_const() and max_const() are expressions so can be used for static > initialisers. I hate the name. Naming shouldn't be about an implementation detail, particularly not an esoteric one like the "C constant expression" rule. That can be useful for some internal helper functions or macros, but not for something that random people are supposed to USE. Telling some random developer that inside an array size declaration or a static initializer you need to use "max_const()" because it needs to syntactically be a constant expression, and our regular "max()" function isn't that, is just *horrid*. No, please just use the traditional C model of just using ALL CAPS for macro names that don't act like a function. Yes, yes, that may end up requiring getting rid of some current users of #define MIN(a,b) ((a)<(b) ? (a):(b)) but dammit, we don't actually have _that_ many of them, and why should we have random drivers doing that anyway? Linus
From: Linus Torvalds > Sent: 25 February 2024 17:14 > > On Sun, 25 Feb 2024 at 08:53, David Laight <David.Laight@aculab.com> wrote: > > > > The expansions of min() and max() contain statement expressions so are > > not valid for static intialisers. > > min_const() and max_const() are expressions so can be used for static > > initialisers. > > I hate the name. Picking name is always hard... > Naming shouldn't be about an implementation detail, particularly not > an esoteric one like the "C constant expression" rule. That can be > useful for some internal helper functions or macros, but not for > something that random people are supposed to USE. > > Telling some random developer that inside an array size declaration or > a static initializer you need to use "max_const()" because it needs to > syntactically be a constant expression, and our regular "max()" > function isn't that, is just *horrid*. > > No, please just use the traditional C model of just using ALL CAPS for > macro names that don't act like a function. > > Yes, yes, that may end up requiring getting rid of some current users of > > #define MIN(a,b) ((a)<(b) ? (a):(b)) > > but dammit, we don't actually have _that_ many of them, and why should > we have random drivers doing that anyway? I'll have a look at what is there. It might take a three part patch though. Unless you apply it as a tree-wide patch? One option is to add as max_const(), then change any existing MAX() to be max() or max_const() and then finally rename to MAX()? David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
... > Yes, yes, that may end up requiring getting rid of some current users of > > #define MIN(a,b) ((a)<(b) ? (a):(b)) > > but dammit, we don't actually have _that_ many of them, and why should > we have random drivers doing that anyway? They look like they could be changed to min(). It is even likely the header gets pulled in somewhere. I'm not sure about the ones in drivers/gpu/drm/amd/display/*..*/*.c, but it wouldn't surprise me if that code doesn't use any standard kernel headers. Isn't that also the code that manages to pass 42 integer parameters to functions? David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Mon, 26 Feb 2024 at 07:26, David Laight <David.Laight@aculab.com> wrote: > > ... > > Yes, yes, that may end up requiring getting rid of some current users of > > > > #define MIN(a,b) ((a)<(b) ? (a):(b)) > > > > but dammit, we don't actually have _that_ many of them, and why should > > we have random drivers doing that anyway? > > They look like they could be changed to min(). > It is even likely the header gets pulled in somewhere. > > I'm not sure about the ones in drivers/gpu/drm/amd/display/*..*/*.c, but it > wouldn't surprise me if that code doesn't use any standard kernel headers. > Isn't that also the code that manages to pass 42 integer parameters > to functions? They are all separate in C files, I think we could get rid of them pretty easily in favour of the standard ones, Adding Harry to cc. Dave.
diff --git a/include/linux/minmax.h b/include/linux/minmax.h index 278a390b8a4c..c08916588425 100644 --- a/include/linux/minmax.h +++ b/include/linux/minmax.h @@ -60,19 +60,34 @@ #op "(" #x ", " #y ") signedness error, fix types or consider u" #op "() before " #op "_t()"); \ __cmp_once(op, x, y, uniq); })) +#define __careful_cmp_const(op, x, y) \ + (BUILD_BUG_ON_ZERO(!__is_constexpr((x) - (y))) + \ + BUILD_BUG_ON_ZERO(!__types_ok(x, y)) + \ + __cmp(op, (x) + 0, (y) + 0)) + /** * min - return minimum of two values of the same or compatible types * @x: first value * @y: second value + * + * If @x and @y are constants the return value is constant, but not 'constant + * enough' for things like static initialisers. + * min_const(@x, @y) is a constant expression for constant inputs. */ #define min(x, y) __careful_cmp(min, x, y, __COUNTER__) +#define min_const(x, y) __careful_cmp_const(min, x, y) /** * max - return maximum of two values of the same or compatible types * @x: first value * @y: second value + * + * If @x and @y are constants the return value is constant, but not 'constant + * enough' for things like static initialisers. + * max_const(@x, @y) is a constant expression for constant inputs. */ #define max(x, y) __careful_cmp(max, x, y, __COUNTER__) +#define max_const(x, y) __careful_cmp_const(max, x, y) /** * umin - return minimum of two non-negative values
The expansions of min() and max() contain statement expressions so are not valid for static intialisers. min_const() and max_const() are expressions so can be used for static initialisers. The arguments are checked for being constant and for negative signed values being converted to large unsigned values. Using these to size on-stack arrays lets min/max be simplified. Zero is added before the compare to convert enum values to integers avoinding the need for casts when enums have been used for constants. Signed-off-by: David Laight <david.laight@aculab.com> --- include/linux/minmax.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+) Changes for v2: - Typographical and spelling corrections to the commit messages. Patches unchanged.