| Message ID | c5870ce0fd1a4a45b11b687705fe8837b71025b0.1438884611.git.DirtY.iCE.hu@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Do, 2015-08-06 at 20:28 +0200, Kővágó, Zoltán wrote: > Currently the gcc specific version only evaluates the arguments once, > while the generic version evaluates one argument twice, which can cause > debugging headaches when an argument has a side effect. The answer to that is "don't do that". Do we have macro calls with side effects in the tree? > This patch at least provides consistent behavior between compilers. Makes sense. > -#else > #define audio_MIN(a, b) ((a)>(b)?(b):(a)) > #define audio_MAX(a, b) ((a)<(b)?(b):(a)) > -#endif include/qemu/osdep.h already provides MIN/MAX macros. I think we should either define audio_MIN (and audio_MAX) to those, or simply do s/audio_MIN/MIN/ in audio/*.c cheers, Gerd
On 19 August 2015 at 19:17, Gerd Hoffmann <kraxel@redhat.com> wrote: > On Do, 2015-08-06 at 20:28 +0200, Kővágó, Zoltán wrote: >> Currently the gcc specific version only evaluates the arguments once, >> while the generic version evaluates one argument twice, which can cause >> debugging headaches when an argument has a side effect. > > The answer to that is "don't do that". Do we have macro calls with side > effects in the tree? > >> This patch at least provides consistent behavior between compilers. > > Makes sense. > >> -#else >> #define audio_MIN(a, b) ((a)>(b)?(b):(a)) >> #define audio_MAX(a, b) ((a)<(b)?(b):(a)) >> -#endif > > include/qemu/osdep.h already provides MIN/MAX macros. > > I think we should either define audio_MIN (and audio_MAX) to those, or > simply do s/audio_MIN/MIN/ in audio/*.c My vote is for the latter. Incidentally we already assume both typeof and statement expr support in our compilers, so we could upgrade our local MIN/MAX implementations to use them if we really needed. (We'd have to rename them though, since the system implementation likely does the eval-twice thing.) A quick grep doesn't show any audio_MIN/MAX which need to avoid multiple-evaluation, though. thanks -- PMM
Hi On Thu, Aug 6, 2015 at 8:28 PM, Kővágó, Zoltán <dirty.ice.hu@gmail.com> wrote: > Currently the gcc specific version only evaluates the arguments once, > while the generic version evaluates one argument twice, which can cause > debugging headaches when an argument has a side effect. This patch at > least provides consistent behavior between compilers. > Going this way, you could simply replace audio_MIN/MAX with MIN/MAX (from osdep.h and glib headers) Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com> > --- > audio/audio.h | 14 -------------- > 1 file changed, 14 deletions(-) > > diff --git a/audio/audio.h b/audio/audio.h > index 68545b6..3a54e17 100644 > --- a/audio/audio.h > +++ b/audio/audio.h > @@ -150,22 +150,8 @@ 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 > > int wav_start_capture(AudioState *state, CaptureState *s, const char *path, > int freq, int bits, int nchannels); > -- > 2.4.5 > >
diff --git a/audio/audio.h b/audio/audio.h index 68545b6..3a54e17 100644 --- a/audio/audio.h +++ b/audio/audio.h @@ -150,22 +150,8 @@ 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 int wav_start_capture(AudioState *state, CaptureState *s, const char *path, int freq, int bits, int nchannels);
Currently the gcc specific version only evaluates the arguments once, while the generic version evaluates one argument twice, which can cause debugging headaches when an argument has a side effect. This patch at least provides consistent behavior between compilers. Signed-off-by: Kővágó, Zoltán <DirtY.iCE.hu@gmail.com> --- audio/audio.h | 14 -------------- 1 file changed, 14 deletions(-)