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

Sun Jan 31 12:35:18 PST 2021

rarutyun marked 4 inline comments as done.
rarutyun added a comment.

> 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.

Done. I decided to do it for copy-constructor and both `volatile` and non-`volatile` overload of copy-assignment. And the same is done for pointer specialization for all three methods.

================
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
----------------
ldionne wrote:
> 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.
Ok, I see your point. I also don't have the strong opinion on that. That's basically what I am doing usually for unobvious fix but probably `git blame` is one of the options.

Anyway, comment has been removed

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90968