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

[Philippe Mathieu-Daudé]

On 12/24/18 9:48 PM, Kővágó Zoltán wrote:
> 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.

You haven't found any info because the minimum requirements are not well
documented on the wiki.

> 
> 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.

Sounds good. Now the parentheses around a/b are not necessary.

I wonder however if it wouldn't be cleaner to previously add:

#ifdef MIN
# undef MIN
#endif

and for MAX, rather than only use these macros if undefs.

> 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.

Fine with me!

If you have this patch ready, you can send it alone out of this series,
no need to resend this whole series for this patch.

Thanks,

Phil.