On 1/4/19 6:23 PM, Zoltán Kővágó wrote: > Hi, > > I have a similar patch in my queue[1] >
Sorry for not noticing that thread. >> #ifndef MIN >> -#define MIN(a, b) (((a) < (b)) ? (a) : (b)) The old version is at least named in ALL_CAPS, to warn the user that it is a macro and may have side effects due to multiple evaluations. >> +#define MIN(a, b) \ >> + ({ \ >> + __auto_type _a = (a); \ >> + __auto_type _b = (b); \ >> + _a < _b ? _a : _b; \ >> + }) >> #endif > I tried this[2], but apparently random linux headers define their own > MIN/MAX and in case this version won't be used. Indeed; changing #ifndef MIN to #undef MIN forces us to use our version rather than inheriting something, at which point... > And the version above > with __auto_type and statement expression doesn't work on bitfields and > when not in functions (for example struct XHCIState has > USBPort uports[MAX(MAXPORTS_2, MAXPORTS_3)]; > as one of its member). Yeah, I ran into that on libvirt as well. It's a real bummer that __auto_type can't be used in constant expressions; I don't know if C11 generics can be used to come up with an alternative that still qualifies as a constant expression. Quoting your other mail: >> /home/dirty_ice/qemu/include/qemu/osdep.h:240:5: error: ‘__auto_type’ >> used with a bit-field initializer >> /home/dirty_ice/qemu/include/qemu/osdep.h:247:35: error: braced-group >> within expression allowed only inside a function >> >> The first one is fixable with an explicit cast (ugly but works), Or by writing: __auto_type _a = (a) + 0; typeof((a) + 0) _a = (a) + 0; to force type promotion of the bitfield 'a' (no casts needed, either at the caller or in the macro). Since we are doing ?: between a and b, we end up with integer promotion anyways (that is, MIN(char, char) still results in int, and that's unchanged regardless of naive or smart implementation of the macro; the only way to get MIN(char, char) to result in char is with typeof, although I don't see any strong reasons why making the macro return an unpromoted value would ever be useful). >> but the >> second one is more problematic. It means we can't write stuff like >> >> USBPort uports[MAX(MAXPORTS_2, MAXPORTS_3)]; >> >> when not in a function. So we either need a dumb version of MIN/MAX, or >> scrape the idea altogether. I'm fine keeping the name MIN/MAX for the common use, but we'd need a second pair, maybe named MIN_CONST/MAX_CONST, for use in contexts that require a constant (there, both arguments are evaluated twice because it is the naive implementation, but that is harmless because both arguments are constant and the macro is then usable in places where the smart MIN/MAX are not). I don't know if trying to use __builtin_constant_p in the const version would also be worthwhile. In fact, if __builtin_constant_p is powerful enough, perhaps we could use it to force a single macro to expand to the naive version if both arguments are constant and the smart version otherwise. I'll give that a shot. > It only works because currently glib/gmacros.h or > sys/param.h defines it's own MIN/MAX which is identical to the old version. > > Now that I think about it, instead of undefining the old macro or only > conditionally defining it, maybe the best course of action would be to > rename MIN/MAX to something more unique so it won't clash with random > system headers. No, if we want smart MIN/MAX, we merely undefine whatever random junk got inherited from the system. If we rename anything, it should be the constant version for use where the smart version is semantically prohibited. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature