Re: [Qemu-devel] [RFC PATCH] osdep: Make MIN/MAX evaluate arguments only once

[Eric Blake]

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