[libcxx-commits] [PATCH] D90968: Fix for the Bug 41784

[Ruslan Arutyunyan via Phabricator via libcxx-commits]

rarutyun added inline comments.


================
Comment at: libcxx/include/atomic:1687
     _LIBCPP_INLINE_VISIBILITY
     __atomic_base(const __atomic_base&);
 #endif
----------------
ldionne wrote:
> Do we have the exact same issue with copy-construction? The following works, but I don't think it should:
> 
> ```
> #include <atomic>
> 
> int main(int, char**) {
>     volatile std::atomic<int> a1;
>     std::atomic<int> a2(a1);
> }
> ```
> 
The answer is weird:) Basically, we have and we don't. Yes, the code you provided works *and* it probably should.

In C++ standard for copy-assignment operator we have two overloads. One of them as explicitly marked `volatile`. Thus, for the test I've added the overload resolution should prefer copy-assignment volatile overload to everything else.

For the constructors we cannot have the overloads with cv-qualified for `*this` because `*this` doesn't exist yet (to my understanding). In C++ standard we have `atomic (const atomic&) = delete;` and that's it. If it's argument of copy-constructor is `atomic` or `const atomic` then it would choose deleted copy-constructor. `volatile atomic` cannot be matched to `const atomic&`. Possible fix is `atomic (const volatile atomic&) = delete;`

So, probably the issue is there but we should bring it to C++ standard committee if we believe this is the issue we need to fix. Currently everything works according to the spec. If the possible fix I wrote above would be taken in C++ standard then you are right and we would need to write the copy constructor in the most derived class instead of `atomic_base`



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

https://reviews.llvm.org/D90968