[libcxx-commits] [PATCH] D103983: [libc++] Improve atomic_fetch_(add|sub).*.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 5 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/include/atomic:1851
     _LIBCPP_INLINE_VISIBILITY
-    _Tp* fetch_add(ptrdiff_t __op, memory_order __m = memory_order_seq_cst)
-                                                                        volatile _NOEXCEPT
+    typename enable_if<is_same<_Tp, _Up>::value && is_object<_Up>::value, _Up*>::type
+    fetch_add(ptrdiff_t __op, memory_order __m = memory_order_seq_cst) volatile _NOEXCEPT
----------------
Mordante wrote:
> Quuxplusone wrote:
> > Mordante wrote:
> > > I'm not too happy with this way to constrain the function. I'm open to better suggestions.
> > I recommend:
> > ```
> > _LIBCPP_INLINE_VISIBILITY
> > _Tp* fetch_add(ptrdiff_t __op, memory_order __m = memory_order_seq_cst) volatile _NOEXCEPT
> >     {return __cxx_atomic_fetch_add(&this->__a_, __op, __m);}
> > ```
> > This follows the Standard's specification exactly, and also is simple (no weird metaprogramming involved). Also, this is what libstdc++ does. (MSVC essentially handles this via a partial specialization: they `conditional_t` between `_Atomic_pointer` and `_Atomic_storage` depending on whether the pointer is an object pointer type. But we don't need to do that, the standard doesn't do that, and libstdc++ doesn't do that. Let's Keep It Simple.)
> > 
> > If you want to explicitly nod to the "Mandates:" in http://eel.is/c++draft/atomics.types.pointer#5 , then I recommend
> > ```
> > _LIBCPP_INLINE_VISIBILITY
> > _Tp* fetch_add(ptrdiff_t __op, memory_order __m = memory_order_seq_cst) volatile _NOEXCEPT {
> >     static_assert(is_object<typename remove_pointer<_Tp>::type>::value, "Must be an object pointer type");
> >     return __cxx_atomic_fetch_add(&this->__a_, __op, __m);
> > }
> > ```
> I'll have a look at the other two implementations. However we need to do something else we do arithmetic on a function pointer. Or do you mean that Clang should protect us against that behavior? When I don't disable the function pointer here it allows function pointers.
I did some testing. Clang accepts pointers to functions, so we need an `static_assert` to guard against these. The other invalid pointers generate a compiler diagnostic.


================
Comment at: libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_fetch_sub_explicit.verify.cpp:26
+    volatile std::atomic<void*> obj;
+    // expected-error at atomic:* {{no matching member function for call to 'fetch_sub'}}
+    std::atomic_fetch_sub_explicit(&obj, 0, std::memory_order_relaxed);
----------------
Mordante wrote:
> Quuxplusone wrote:
> > .verify.cpp tests are run only on libc++? This test wouldn't pass against libstdc++, for example, because libstdc++ doesn't SFINAE away `fetch_sub` in these cases. (There'd still be an error; it'd just be a //different// error, because subtracting from a `void*` is ill-formed.)
> Correct these tests are only executed with Clang and libc++. So here it's valid to validate against Clang specific diagnostics.
I recently learned these still should be in the libcxx instead of the std part of the tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103983/new/

https://reviews.llvm.org/D103983