[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [RFC PATCH] maint: let MIN/MAX evaluate only once, on modern compilers



On Fri, Jan 04, 2019 at 10:00:12 -0600, Eric Blake wrote:
> We might as well take advantage of gcc's extensions for a safer
> MIN()/MAX() macro.
> 
> Signed-off-by: Eric Blake <eblake redhat com>
> ---
> 
> RFC because we could also try to fall back to older gcc's typeof(a)_a=(a)
> when the newer __auto_type _a=(a) is not present, and because I don't
> know how to properly probe for __auto_type and/or typeof support in
> clang (I know that clang added __auto_type in 2015, per
> https://reviews.llvm.org/D12686); I also don't know if we have officially
> stated that we require gcc/clang (because of attribute((cleanup)), which
> really cannot be emulated without extensions), or if we still try to
> allow compilation with other compilers; if we have, I don't know if we
> have declared a minimum compiler version that we demand (qemu has recently
> demanded gcc 4.8 or clang 3.4 [branded 5.1 on Apple]).
> 
>  src/util/virutil.h | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/src/util/virutil.h b/src/util/virutil.h
> index 908d8920ec..364e354bcb 100644
> --- a/src/util/virutil.h
> +++ b/src/util/virutil.h
> @@ -28,10 +28,26 @@
>  # include <sys/types.h>
> 
>  # ifndef MIN
> -#  define MIN(a, b) ((a) < (b) ? (a) : (b))
> +#  if __GNUC_PREREQ(4, 9)
> +#   define MIN(a, b) ({                         \
> +            __auto_type _a = (a);               \
> +            __auto_type _b = (b);               \
> +            _a < _b ? _a : _b;                  \
> +        })
> +#  else
> +#   define MIN(a, b) ((a) < (b) ? (a) : (b))
> +#  endif
>  # endif
>  # ifndef MAX
> -#  define MAX(a, b) ((a) > (b) ? (a) : (b))
> +#  if __GNUC_PREREQ(4, 9)
> +#   define MAX(a, b) ({                         \
> +            __auto_type _a = (a);               \
> +            __auto_type _b = (b);               \
> +            _a > _b ? _a : _b;                  \
> +        })
> +#  else
> +#   define MAX(a, b) ((a) > (b) ? (a) : (b))
> +#  endif

I think we don't really need this patch at all. It makes sure the
arguments are only evaluated once, but since this is all conditional
(not only the safer variants, but the implementation can even be
provided externally), we still have to care what arguments we pass to
MIN/MAX.

In other words, I think this actually reduces the safety of our code.

Jirka


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]