From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54684) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gcV6h-0006da-RT for qemu-devel@nongnu.org; Thu, 27 Dec 2018 07:49:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gcV6g-0001Q0-PF for qemu-devel@nongnu.org; Thu, 27 Dec 2018 07:49:47 -0500 Received: from mail-wr1-x432.google.com ([2a00:1450:4864:20::432]:34581) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1gcV6g-0001OP-I2 for qemu-devel@nongnu.org; Thu, 27 Dec 2018 07:49:46 -0500 Received: by mail-wr1-x432.google.com with SMTP id j2so18246249wrw.1 for ; Thu, 27 Dec 2018 04:49:46 -0800 (PST) From: "=?UTF-8?B?S8WRdsOhZ8OzIFpvbHTDoW4=?=" References: <11a6562f583531e5a5473716bea44ee3ae7be120.1545598229.git.DirtY.iCE.hu@gmail.com> <66e793ef-dd32-eb81-14f5-cf59ca8c73bb@amsat.org> <980b862a-9c2d-8ab7-2937-846524399148@gmail.com> <42f870e2-97bc-1028-3587-d9ae31537a13@amsat.org> <20a6e023-89be-f2e1-aa11-0686a1dfe021@gmail.com> <3ff67b65-935e-9149-05a7-543752723b06@amsat.org> Message-ID: Date: Thu, 27 Dec 2018 13:49:43 +0100 MIME-Version: 1.0 In-Reply-To: <3ff67b65-935e-9149-05a7-543752723b06@amsat.org> Content-Type: text/plain; charset=utf-8 Content-Language: hu-HU Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v2 23/52] audio: remove audio_MIN, audio_MAX List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , qemu-devel@nongnu.org, Paolo Bonzini Cc: Michael Walle , Gerd Hoffmann On 2018-12-25 11:40, Philippe Mathieu-Daudé wrote: > 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 >>>>>> >>>>>> --- >>>>>> >>>>>> 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. Um, this broke a few things. It also means until now in some cases it used some random definition of MIN/MAX instead of the one in the header. /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), 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. > >> 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. There will be adjustmets to other patches too, so I think I'll just consolidate them. Regards, Zoltan