Re: [Qemu-devel] [PATCH v2 23/52] audio: remove audio_MIN, audio_MAX

[Kővágó Zoltán]

On 2018-12-24 18:16, Philippe Mathieu-Daudé wrote:
> On 12/24/18 3:16 AM, Zoltán Kővágó wrote:
>> Hi Phil,
>>
>> On 2018-12-24 00:49, Philippe Mathieu-Daudé wrote:
>>> Hi Zoltán,
>>>
>>> On 12/23/18 9:51 PM, Kővágó, Zoltán wrote:
>>>> There's already a MIN and MAX macro in include/qemu/osdep.h, use them
>>>> instead.
>>>>
>>>> Signed-off-by: Kővágó, Zoltán <address@hidden>
>>>>
>>>> ---
>>>>
>>>> Changes from v1:
>>>> * removed audio_MIN, audio_MAX macros
>>>> ---
>>> [...]>
>>>> diff --git a/audio/audio.h b/audio/audio.h
>>>> index ccfef9e10a..bcbe56d639 100644
>>>> --- a/audio/audio.h
>>>> +++ b/audio/audio.h
>>>> @@ -148,23 +148,6 @@ static inline void *advance (void *p, int incr)
>>>>      return (d + incr);
>>>>  }
>>>>  
>>>> -#ifdef __GNUC__
>>>> -#define audio_MIN(a, b) ( __extension__ ({      \
>>>> -    __typeof (a) ta = a;                        \
>>>> -    __typeof (b) tb = b;                        \
>>>> -    ((ta)>(tb)?(tb):(ta));                      \
>>>> -}))
>>>> -
>>>> -#define audio_MAX(a, b) ( __extension__ ({      \
>>>> -    __typeof (a) ta = a;                        \
>>>> -    __typeof (b) tb = b;                        \
>>>> -    ((ta)<(tb)?(tb):(ta));                      \
>>>> -}))
>>>> -#else
>>>> -#define audio_MIN(a, b) ((a)>(b)?(b):(a))
>>>> -#define audio_MAX(a, b) ((a)<(b)?(b):(a))
>>>> -#endif
>>>> -
>>>
>>> Those MIN/MAX are smarter than the ones in "qemu/osdep.h", I'd keep them
>>> moving them there.
>>
>> Yes, but only when using gcc (or clang when it emulates gcc).
>> Unfortunately, they work differently when one of the expressions has
>> side effects, which is a disaster waiting to happen (when some poor folk
>> finally tries to compile it with a non-gcc compiler).
> 
> We already use the typeof extension:
> 
> $ git grep typeof|wc -l
> 145
> 
> For MIN/MAX I'd use rather use the __auto_type extension.
> 
>> Or do we support any compilers beside gcc and clang? Because if not,
>> sure, do it, just remove the #ifdef and keep only the gcc version.
> 
> Yes, this is checked by the ./configure script:
> 
> 1850 # Check whether the compiler matches our minimum requirements:
> ...
> 1859 #   error You need at least Clang v3.4 to compile QEMU
> ...
> 1864 #  error You need at least GCC v4.8 to compile QEMU
> ...
> 1867 # error You either need GCC or Clang to compiler QEMU

Fair enough. I've tried to check the documentation for supported
compilers, but I haven't found any info. Well, I didn't look at the
configure script, I admit that.

This is what I came up with:

#ifndef MIN
#define MIN(a, b) ( __extension__ ({      \
    __auto_type _a = (a);                 \
    __auto_type _b = (b);                 \
    _a > _b ? _b : _a;                    \
}))
#endif
#ifndef MAX
#define MAX(a, b) ( __extension__ ({      \
    __auto_type _a = (a);                 \
    __auto_type _b = (b);                 \
    _a < _b ? _b : _a;                    \
}))
#endif


I changed it a bit, since a and b are the macro parameters, so they are
the variables that we should put into parentheses, and I renamed ta/tb
to _a/_b, this way they're less likely to clobber some other variables.

To be honest, we could probably drop the __extension__ keyword too,
we're not compiling with -pedantic. Other than audio_MIN and audio_MAX,
it only appears in the DO_UPCAST macro.

Regards,
Zoltan