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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D90968#2521256 <https://reviews.llvm.org/D90968#2521256>, @rarutyun wrote:

> In D90968#2507523 <https://reviews.llvm.org/D90968#2507523>, @ldionne wrote:
>
>> In addition to the comments I made, can you please add tests using the type traits like `static_assert(!std::is_copy_constructible_v<std::atomic<int>>);`?
>
> Sure, will do it in the next update. I guess I should check both copy constructible and copy assignable there. Anything else?

That's all I can think of. We could have checked for `!std::is_constructible<std::atomic<T>, std::atomic<T> const volatile&>`, but as you say, the spec seems to allow it right now.



================
Comment at: libcxx/include/atomic:1687
     _LIBCPP_INLINE_VISIBILITY
     __atomic_base(const __atomic_base&);
 #endif
----------------
rarutyun wrote:
> 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`
> 
Ah, I see. So we're calling `atomic::operator T()` on `a1`, and then `atomic<T>::atomic(T desired)` to construct `a2`. @__simt__ Do you think this is expected, or should an issue be filed?

It certainly violates what I would have naively expected.


================
Comment at: libcxx/include/atomic:1798
+
+    // delete explicitly in the most derived class to explicitly in the most derived class to satisfy
+    // the requirement that atomic (possibly volatile qualified) variables are not copyable
----------------
rarutyun wrote:
> ldionne wrote:
> > I don't feel like this comment adds something useful.
> The mess in the comments was fixed. Or do you suggest something else? Should I remove the comment and just leave the reference to the bug or should I completely remove the comment?
I don't think the comment (even fixed) adds much value, but feel free to leave it there if you prefer. If someone's interested in knowing why we inherit here, they can `git blame`. Another way to see this -- if this had been written that way from the start (and a bug report never had to be filed), would this comment be there? I don't believe so. That's sort of the point I'm trying to make.

But as I said, if you think it's useful, feel free to keep it. I'm not strongly attached either way, it's just a comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90968