[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:2172-2177
 typename enable_if
 <
-    is_integral<_Tp>::value && !is_same<_Tp, bool>::value && !is_const<_Tp>::value,
+    (is_integral<_Tp>::value && !is_same<_Tp, bool>::value && !is_const<_Tp>::value) ||
+	(is_pointer<_Tp>::value && is_object<typename remove_pointer<_Tp>::type>::value && !is_const<_Tp>::value),
     _Tp
 >::type
----------------
IMO, libc++ is wrong to constrain these functions at all.
https://eel.is/c++draft/atomics.nonmembers
does not mention any constraints. The only requirement on `_Tp` here is that `typename atomic<_Tp>::difference_type` must exist.

So, I would simply eliminate the `enable_if`s here. Write
```
template <class _Tp>
_LIBCPP_INLINE_VISIBILITY
_Tp atomic_fetch_add(volatile atomic<_Tp>* __o, typename atomic<_Tp>::difference_type __op) _NOEXCEPT
{
    return __o->fetch_add(__op);
}
```
to match the Standard specification exactly.

Compare to `atomic_store` on line 1908, for example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103983