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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked an inline comment as done.
Mordante added a comment.

Thanks for the review!



================
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
----------------
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.


================
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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103983