Re: [Mesa-dev] [PATCH] util/u_atomic: use STATIC_ASSERT() when selecting the appropriate function
On 13 May 2015 at 21:01, Brian Paul wrote: > On 05/13/2015 01:18 PM, Francisco Jerez wrote: >> >> Emil Velikov writes: >> >>> As we evaluate sizeof() at compile time, having the run-time assert() >>> does not provide any benefits. Move to the compile-time version >>> STATIC_ASSERT() which will kindly prompt us when this go wrong. >>> >> >> This doesn't look right. AFAIK STATIC_ASSERT() is implemented by >> expanding to an array type-id of negative size, which is an error >> regardless of whether the sizeof expression is evaluated or not: i.e. >> "0 ? sizeof(invalid) : ..." is still an error for the same reason that >> "0 * sizeof(invalid)" or "(void)sizeof(invalid)" is an error. That and >> also that the whole thing is wrapped in a do-while loop you cannot use >> as operand of the ternary operator... > > > Yeah, this does not build with MSVC: > > C:\Users\Brian\projects\mesa\src\gallium\auxiliary\util/u_inlines.h(83) : > error C2059: syntax error : 'do' > C:\Users\Brian\projects\mesa\src\gallium\auxiliary\util/u_inlines.h(83) : > error C2143: syntax error : missing ';' before ',' > C:\Users\Brian\projects\mesa\src\gallium\auxiliary\util/u_inlines.h(83) : > error C2059: syntax error : ')' > C:\Users\Brian\projects\mesa\src\gallium\auxiliary\util/u_inlines.h(89) : > error C2059: syntax error : 'do' > [...] > This one goes straight into the bitbin. Thanks for the explanation Francisco and test Brian. -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] util/u_atomic: use STATIC_ASSERT() when selecting the appropriate function
On 05/13/2015 01:18 PM, Francisco Jerez wrote: Emil Velikov writes: As we evaluate sizeof() at compile time, having the run-time assert() does not provide any benefits. Move to the compile-time version STATIC_ASSERT() which will kindly prompt us when this go wrong. This doesn't look right. AFAIK STATIC_ASSERT() is implemented by expanding to an array type-id of negative size, which is an error regardless of whether the sizeof expression is evaluated or not: i.e. "0 ? sizeof(invalid) : ..." is still an error for the same reason that "0 * sizeof(invalid)" or "(void)sizeof(invalid)" is an error. That and also that the whole thing is wrapped in a do-while loop you cannot use as operand of the ternary operator... Yeah, this does not build with MSVC: C:\Users\Brian\projects\mesa\src\gallium\auxiliary\util/u_inlines.h(83) : error C2059: syntax error : 'do' C:\Users\Brian\projects\mesa\src\gallium\auxiliary\util/u_inlines.h(83) : error C2143: syntax error : missing ';' before ',' C:\Users\Brian\projects\mesa\src\gallium\auxiliary\util/u_inlines.h(83) : error C2059: syntax error : ')' C:\Users\Brian\projects\mesa\src\gallium\auxiliary\util/u_inlines.h(89) : error C2059: syntax error : 'do' [...] -Brian Cc: Matt Turner Signed-off-by: Emil Velikov --- src/util/u_atomic.h | 26 +- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/util/u_atomic.h b/src/util/u_atomic.h index e38395a..10c0cca 100644 --- a/src/util/u_atomic.h +++ b/src/util/u_atomic.h @@ -86,7 +86,7 @@ #endif #include #include -#include +#include "util/macros.h" #if _MSC_VER < 1600 @@ -166,7 +166,7 @@ _InterlockedExchangeAdd8(char volatile *addend, char value) sizeof *(_v) == sizeof(short) ? _InterlockedIncrement16((short *) (_v)) : \ sizeof *(_v) == sizeof(long)? _InterlockedIncrement ((long *) (_v)) : \ sizeof *(_v) == sizeof(__int64) ? InterlockedIncrement64 ((__int64 *)(_v)) : \ - (assert(!"should not get here"), 0)) + (STATIC_ASSERT(!"should not get here"), 0)) #define p_atomic_dec(_v) \ ((void) p_atomic_dec_return(_v)) @@ -175,21 +175,21 @@ _InterlockedExchangeAdd8(char volatile *addend, char value) sizeof *(_v) == sizeof(short) ? _InterlockedDecrement16((short *) (_v)) : \ sizeof *(_v) == sizeof(long)? _InterlockedDecrement ((long *) (_v)) : \ sizeof *(_v) == sizeof(__int64) ? InterlockedDecrement64 ((__int64 *)(_v)) : \ - (assert(!"should not get here"), 0)) + (STATIC_ASSERT(!"should not get here"), 0)) #define p_atomic_add(_v, _i) (\ sizeof *(_v) == sizeof(char)? _InterlockedExchangeAdd8 ((char *) (_v), (_i)) : \ sizeof *(_v) == sizeof(short) ? _InterlockedExchangeAdd16((short *) (_v), (_i)) : \ sizeof *(_v) == sizeof(long)? _InterlockedExchangeAdd ((long *) (_v), (_i)) : \ sizeof *(_v) == sizeof(__int64) ? InterlockedExchangeAdd64((__int64 *)(_v), (_i)) : \ - (assert(!"should not get here"), 0)) + (STATIC_ASSERT(!"should not get here"), 0)) #define p_atomic_cmpxchg(_v, _old, _new) (\ sizeof *(_v) == sizeof(char)? _InterlockedCompareExchange8 ((char *) (_v), (char) (_new), (char) (_old)) : \ sizeof *(_v) == sizeof(short) ? _InterlockedCompareExchange16((short *) (_v), (short) (_new), (short) (_old)) : \ sizeof *(_v) == sizeof(long)? _InterlockedCompareExchange ((long *) (_v), (long) (_new), (long) (_old)) : \ sizeof *(_v) == sizeof(__int64) ? InterlockedCompareExchange64 ((__int64 *)(_v), (__int64)(_new), (__int64)(_old)) : \ - (assert(!"should not get here"), 0)) + (STATIC_ASSERT(!"should not get here"), 0)) #endif @@ -198,7 +198,7 @@ _InterlockedExchangeAdd8(char volatile *addend, char value) #define PIPE_ATOMIC "Solaris OS atomic functions" #include -#include +#include "util/macros.h" #define p_atomic_set(_v, _i) (*(_v) = (_i)) #define p_atomic_read(_v) (*(_v)) @@ -208,49 +208,49 @@ _InterlockedExchangeAdd8(char volatile *addend, char value) sizeof(*v) == sizeof(uint16_t) ? atomic_dec_16_nv((uint16_t *)(v)) == 0 : \ sizeof(*v) == sizeof(uint32_t) ? atomic_dec_32_nv((uint32_t *)(v)) == 0 : \ sizeof(*v) == sizeof(uint64_t) ? atomic_dec_64_nv((uint64_t *)(v)) == 0 : \ -(assert(!"should not get here"), 0)) +(STATIC_ASSERT(!"should not get here"), 0)) #define p_atomic_inc(v) (void) (\ sizeof(*v) == sizeof(uint8_t) ? atomic_inc_8 ((uint8_t *)(v)) : \ sizeof(*v) == sizeof(uint16_t) ? atomic_inc_16((uint16_t *)(v)) : \ sizeof(*v) == sizeof(uint32_t) ? atomic_inc_32((uint32_t *)(v)) : \ sizeof(*v) =
Re: [Mesa-dev] [PATCH] util/u_atomic: use STATIC_ASSERT() when selecting the appropriate function
Emil Velikov writes: > As we evaluate sizeof() at compile time, having the run-time assert() > does not provide any benefits. Move to the compile-time version > STATIC_ASSERT() which will kindly prompt us when this go wrong. > This doesn't look right. AFAIK STATIC_ASSERT() is implemented by expanding to an array type-id of negative size, which is an error regardless of whether the sizeof expression is evaluated or not: i.e. "0 ? sizeof(invalid) : ..." is still an error for the same reason that "0 * sizeof(invalid)" or "(void)sizeof(invalid)" is an error. That and also that the whole thing is wrapped in a do-while loop you cannot use as operand of the ternary operator... > Cc: Matt Turner > Signed-off-by: Emil Velikov > --- > src/util/u_atomic.h | 26 +- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/src/util/u_atomic.h b/src/util/u_atomic.h > index e38395a..10c0cca 100644 > --- a/src/util/u_atomic.h > +++ b/src/util/u_atomic.h > @@ -86,7 +86,7 @@ > #endif > #include > #include > -#include > +#include "util/macros.h" > > #if _MSC_VER < 1600 > > @@ -166,7 +166,7 @@ _InterlockedExchangeAdd8(char volatile *addend, char > value) > sizeof *(_v) == sizeof(short) ? _InterlockedIncrement16((short *) > (_v)) : \ > sizeof *(_v) == sizeof(long)? _InterlockedIncrement ((long *) > (_v)) : \ > sizeof *(_v) == sizeof(__int64) ? InterlockedIncrement64 ((__int64 > *)(_v)) : \ > - (assert(!"should not get here"), 0)) > + (STATIC_ASSERT(!"should not get here"), > 0)) > > #define p_atomic_dec(_v) \ > ((void) p_atomic_dec_return(_v)) > @@ -175,21 +175,21 @@ _InterlockedExchangeAdd8(char volatile *addend, char > value) > sizeof *(_v) == sizeof(short) ? _InterlockedDecrement16((short *) > (_v)) : \ > sizeof *(_v) == sizeof(long)? _InterlockedDecrement ((long *) > (_v)) : \ > sizeof *(_v) == sizeof(__int64) ? InterlockedDecrement64 ((__int64 > *)(_v)) : \ > - (assert(!"should not get here"), 0)) > + (STATIC_ASSERT(!"should not get here"), > 0)) > > #define p_atomic_add(_v, _i) (\ > sizeof *(_v) == sizeof(char)? _InterlockedExchangeAdd8 ((char *) > (_v), (_i)) : \ > sizeof *(_v) == sizeof(short) ? _InterlockedExchangeAdd16((short *) > (_v), (_i)) : \ > sizeof *(_v) == sizeof(long)? _InterlockedExchangeAdd ((long *) > (_v), (_i)) : \ > sizeof *(_v) == sizeof(__int64) ? InterlockedExchangeAdd64((__int64 > *)(_v), (_i)) : \ > - (assert(!"should not get here"), 0)) > + (STATIC_ASSERT(!"should not get here"), > 0)) > > #define p_atomic_cmpxchg(_v, _old, _new) (\ > sizeof *(_v) == sizeof(char)? _InterlockedCompareExchange8 ((char *) > (_v), (char) (_new), (char) (_old)) : \ > sizeof *(_v) == sizeof(short) ? _InterlockedCompareExchange16((short *) > (_v), (short) (_new), (short) (_old)) : \ > sizeof *(_v) == sizeof(long)? _InterlockedCompareExchange ((long *) > (_v), (long) (_new), (long) (_old)) : \ > sizeof *(_v) == sizeof(__int64) ? InterlockedCompareExchange64 ((__int64 > *)(_v), (__int64)(_new), (__int64)(_old)) : \ > - (assert(!"should not get here"), 0)) > + (STATIC_ASSERT(!"should not get here"), > 0)) > > #endif > > @@ -198,7 +198,7 @@ _InterlockedExchangeAdd8(char volatile *addend, char > value) > #define PIPE_ATOMIC "Solaris OS atomic functions" > > #include > -#include > +#include "util/macros.h" > > #define p_atomic_set(_v, _i) (*(_v) = (_i)) > #define p_atomic_read(_v) (*(_v)) > @@ -208,49 +208,49 @@ _InterlockedExchangeAdd8(char volatile *addend, char > value) > sizeof(*v) == sizeof(uint16_t) ? atomic_dec_16_nv((uint16_t *)(v)) == 0 : > \ > sizeof(*v) == sizeof(uint32_t) ? atomic_dec_32_nv((uint32_t *)(v)) == 0 : > \ > sizeof(*v) == sizeof(uint64_t) ? atomic_dec_64_nv((uint64_t *)(v)) == 0 : > \ > -(assert(!"should not get here"), 0)) > +(STATIC_ASSERT(!"should not get here"), > 0)) > > #define p_atomic_inc(v) (void) (\ > sizeof(*v) == sizeof(uint8_t) ? atomic_inc_8 ((uint8_t *)(v)) : \ > sizeof(*v) == sizeof(uint16_t) ? atomic_inc_16((uint16_t *)(v)) : \ > sizeof(*v) == sizeof(uint32_t) ? atomic_inc_32((uint32_t *)(v)) : \ > sizeof(*v) == sizeof(uint64_t) ? atomic_inc_64((uint64_t *)(v)) : \ > -(assert(!"should not get here"), 0)) > +(STATIC_ASSERT(!"should not get here"), > 0)) > > #define p_atomic_inc_return(v) ((__typeof(*v)) \ > sizeof(*v) == sizeof(uint8_t) ? atomic_inc_8_nv ((uint8_t *)(v)) : \ > sizeof(*v) == siz
Re: [Mesa-dev] [PATCH] util/u_atomic: use STATIC_ASSERT() when selecting the appropriate function
On 13/05/15 16:43, Matt Turner wrote: > On Wed, May 13, 2015 at 10:37 AM, Emil Velikov > wrote: >> As we evaluate sizeof() at compile time, having the run-time assert() >> does not provide any benefits. Move to the compile-time version >> STATIC_ASSERT() which will kindly prompt us when this go wrong. >> >> Cc: Matt Turner >> Signed-off-by: Emil Velikov >> --- > > Oh, good idea. Presumably you confirmed that the STATIC_ASSERT > triggers at compile-time when you pass an unsupported type to the > macro? If so, > With some hacking[1] it did trigger. Just in case, Vinson - can you try this patch [2] on a Windows/SolarisOS VM please ? > Reviewed-by: Matt Turner > Thanks ! -Emil [1] The asserts are only for the Windows/Solaris code, so I copied the logic to the Linux/GCC build, and dropped on the "sizeof(*v) == sizeof(uint64_t)" case. [2] http://patchwork.freedesktop.org/patch/49235/ ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] util/u_atomic: use STATIC_ASSERT() when selecting the appropriate function
On Wed, May 13, 2015 at 10:37 AM, Emil Velikov wrote: > As we evaluate sizeof() at compile time, having the run-time assert() > does not provide any benefits. Move to the compile-time version > STATIC_ASSERT() which will kindly prompt us when this go wrong. > > Cc: Matt Turner > Signed-off-by: Emil Velikov > --- Oh, good idea. Presumably you confirmed that the STATIC_ASSERT triggers at compile-time when you pass an unsupported type to the macro? If so, Reviewed-by: Matt Turner ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] util/u_atomic: use STATIC_ASSERT() when selecting the appropriate function
As we evaluate sizeof() at compile time, having the run-time assert() does not provide any benefits. Move to the compile-time version STATIC_ASSERT() which will kindly prompt us when this go wrong. Cc: Matt Turner Signed-off-by: Emil Velikov --- src/util/u_atomic.h | 26 +- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/util/u_atomic.h b/src/util/u_atomic.h index e38395a..10c0cca 100644 --- a/src/util/u_atomic.h +++ b/src/util/u_atomic.h @@ -86,7 +86,7 @@ #endif #include #include -#include +#include "util/macros.h" #if _MSC_VER < 1600 @@ -166,7 +166,7 @@ _InterlockedExchangeAdd8(char volatile *addend, char value) sizeof *(_v) == sizeof(short) ? _InterlockedIncrement16((short *) (_v)) : \ sizeof *(_v) == sizeof(long)? _InterlockedIncrement ((long *) (_v)) : \ sizeof *(_v) == sizeof(__int64) ? InterlockedIncrement64 ((__int64 *)(_v)) : \ - (assert(!"should not get here"), 0)) + (STATIC_ASSERT(!"should not get here"), 0)) #define p_atomic_dec(_v) \ ((void) p_atomic_dec_return(_v)) @@ -175,21 +175,21 @@ _InterlockedExchangeAdd8(char volatile *addend, char value) sizeof *(_v) == sizeof(short) ? _InterlockedDecrement16((short *) (_v)) : \ sizeof *(_v) == sizeof(long)? _InterlockedDecrement ((long *) (_v)) : \ sizeof *(_v) == sizeof(__int64) ? InterlockedDecrement64 ((__int64 *)(_v)) : \ - (assert(!"should not get here"), 0)) + (STATIC_ASSERT(!"should not get here"), 0)) #define p_atomic_add(_v, _i) (\ sizeof *(_v) == sizeof(char)? _InterlockedExchangeAdd8 ((char *) (_v), (_i)) : \ sizeof *(_v) == sizeof(short) ? _InterlockedExchangeAdd16((short *) (_v), (_i)) : \ sizeof *(_v) == sizeof(long)? _InterlockedExchangeAdd ((long *) (_v), (_i)) : \ sizeof *(_v) == sizeof(__int64) ? InterlockedExchangeAdd64((__int64 *)(_v), (_i)) : \ - (assert(!"should not get here"), 0)) + (STATIC_ASSERT(!"should not get here"), 0)) #define p_atomic_cmpxchg(_v, _old, _new) (\ sizeof *(_v) == sizeof(char)? _InterlockedCompareExchange8 ((char *) (_v), (char) (_new), (char) (_old)) : \ sizeof *(_v) == sizeof(short) ? _InterlockedCompareExchange16((short *) (_v), (short) (_new), (short) (_old)) : \ sizeof *(_v) == sizeof(long)? _InterlockedCompareExchange ((long *) (_v), (long) (_new), (long) (_old)) : \ sizeof *(_v) == sizeof(__int64) ? InterlockedCompareExchange64 ((__int64 *)(_v), (__int64)(_new), (__int64)(_old)) : \ - (assert(!"should not get here"), 0)) + (STATIC_ASSERT(!"should not get here"), 0)) #endif @@ -198,7 +198,7 @@ _InterlockedExchangeAdd8(char volatile *addend, char value) #define PIPE_ATOMIC "Solaris OS atomic functions" #include -#include +#include "util/macros.h" #define p_atomic_set(_v, _i) (*(_v) = (_i)) #define p_atomic_read(_v) (*(_v)) @@ -208,49 +208,49 @@ _InterlockedExchangeAdd8(char volatile *addend, char value) sizeof(*v) == sizeof(uint16_t) ? atomic_dec_16_nv((uint16_t *)(v)) == 0 : \ sizeof(*v) == sizeof(uint32_t) ? atomic_dec_32_nv((uint32_t *)(v)) == 0 : \ sizeof(*v) == sizeof(uint64_t) ? atomic_dec_64_nv((uint64_t *)(v)) == 0 : \ -(assert(!"should not get here"), 0)) +(STATIC_ASSERT(!"should not get here"), 0)) #define p_atomic_inc(v) (void) (\ sizeof(*v) == sizeof(uint8_t) ? atomic_inc_8 ((uint8_t *)(v)) : \ sizeof(*v) == sizeof(uint16_t) ? atomic_inc_16((uint16_t *)(v)) : \ sizeof(*v) == sizeof(uint32_t) ? atomic_inc_32((uint32_t *)(v)) : \ sizeof(*v) == sizeof(uint64_t) ? atomic_inc_64((uint64_t *)(v)) : \ -(assert(!"should not get here"), 0)) +(STATIC_ASSERT(!"should not get here"), 0)) #define p_atomic_inc_return(v) ((__typeof(*v)) \ sizeof(*v) == sizeof(uint8_t) ? atomic_inc_8_nv ((uint8_t *)(v)) : \ sizeof(*v) == sizeof(uint16_t) ? atomic_inc_16_nv((uint16_t *)(v)) : \ sizeof(*v) == sizeof(uint32_t) ? atomic_inc_32_nv((uint32_t *)(v)) : \ sizeof(*v) == sizeof(uint64_t) ? atomic_inc_64_nv((uint64_t *)(v)) : \ -(assert(!"should not get here"), 0)) +(STATIC_ASSERT(!"should not get here"), 0)) #define p_atomic_dec(v) ((void) \ sizeof(*v) == sizeof(uint8_t) ? atomic_dec_8 ((uint8_t *)(v)) : \ sizeof(*v) == sizeof(uint16_t) ? atomic_dec_16((uint16_t *)(v)) : \ sizeof(*v) == sizeof(uint32_t) ? atomic_dec_32((uint32_t *)(v)) : \ sizeof(*v) == sizeof(uint64_t) ? atomic_dec_64((uint64_t *)(v)) : \ -
