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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone 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:
> 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);
}
```


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103983