[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:2196-2212
+#ifdef DO_WE_WANT_TO_KEEP_THIS_COMPILER_EXTENSION
 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);
----------------
Mordante wrote:
> Quuxplusone wrote:
> > This is not a compiler extension; I mean,
> > ```
> > atomic<int*> a;
> > atomic_fetch_add(&a, 1);
> > ```
> > must keep working no matter what. https://godbolt.org/z/ajdEYPhfK
> > I suggest that you keep it working — and simplify the implementation — by removing the bogus constraints on line 2187, and then delete lines 2196-2212.
> > (I'm willing to commandeer, if what I'm saying isn't making sense to you.)
> > This is not a compiler extension; I mean,
> > ```
> > atomic<int*> a;
> > atomic_fetch_add(&a, 1);
> > ```
> > must keep working no matter what. https://godbolt.org/z/ajdEYPhfK
> 
> Correct, but that's not an extension. These two are https://godbolt.org/z/rs1oxYEMv
> ```
> 	atomic<int*> a;
> 	atomic_fetch_add<int>(&a, 1);
> 
> 	atomic<int(*)()> f;
> 	atomic_fetch_add(&f, 1);
> ```
> 
> > I suggest that you keep it working — and simplify the implementation — by removing the bogus constraints on line 2187, and then delete lines 2196-2212.
> > (I'm willing to commandeer, if what I'm saying isn't making sense to you.)
> 
> What you say makes sense, but I just want to know whether we want to keep these 2 extensions (bugs?).
> There's no need to commandeer, I just want to decide which direction to take before implementing the changes and the tests.
> 
> 
> 
Ah. Yeah, "obviously" it's OK to remove those usages.

- Using `atomic_fetch_add<int>` with an explicit template argument violates the general SD-8-esque rules about explicit template arguments, e.g. `make_pair<int, int>`.

- Incrementing a function pointer doesn't make sense semantically; currently we codegen it to a no-op; it was never supposed to be supported. 

Lines 2196-2212 were clearly just trying to implement `atomic_fetch_add` for pointers (after accidentally/bogusly //failing// to support pointers in the primary template, due to that bogus constraint that needs removing).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103983