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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

I'll look at implementing the proper constrains for the final patch.
For now I would like to determine whether we want to keep or remove the non-standard extension.
(Not sure whether they're extensions or bugs.)



================
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
----------------
Quuxplusone wrote:
> 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.
Agreed it would be better to add the constrains in the class.
This seems to be required already. In libc++ you can add to an atomic function pointer.
https://godbolt.org/z/effMvEr8j
This isn't allowed http://eel.is/c++draft/atomics.types.pointer#5




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103983