mstorsjo added inline comments. ================ Comment at: lib/Headers/intrin.h:504 @@ +503,3 @@ +_interlockedbittestandset_acq(long volatile *_BitBase, long _BitPos) { + long _PrevVal = __atomic_fetch_or(_BitBase, 1l << _BitPos, __ATOMIC_ACQUIRE); + return (_PrevVal >> _BitPos) & 1; ---------------- compnerd wrote: > mstorsjo wrote: > > compnerd wrote: > > > Perhaps we should add static asserts that _BitPos is within limits for > > > the signed shift. > > Sure, although I guess that also goes for the existing inline functions as > > well? > > > > Which kind of assert would be suitable for that here? As far as I see, > > static_assert is C++ only, while this header also can be used from C. > > > > If I try to add _Static_assert, which is usable in C, I get the following > > error when compiling: > > > > intrin.h:499:18: error: > > static_assert expression is not an integral constant expression > > _Static_assert(_BitPos < 32, "_BitPos out of range"); > > > > This even when I don't actually use the inline function anywhere, just > > including intrin.h. > Yeah, we would have to use `_Static_assert`, but that requires C11. It is > possible to emulate it, but probably not worth the effort. I am worried > though that we could introduce undefined behavior with an incorrect parameter. Sure, there's probably such a risk. But this issue already exists - an almost identical _interlockedbittestandset function already exists in the header - I'm just adding new copies of it with different atomic semantics (__ATOMIC_ACQUIRE etc).
So I'd ask if we can deal with that issue separately from this patch. https://reviews.llvm.org/D24609 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits