[libcxx-commits] [PATCH] D102135: [libcxx][ranges] adds _`copyable-box`_

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__ranges/copyable_box.h:13
+
+// These customization variables are used in <span> and <string_view>. The
+// separate header is used to avoid including the entire <ranges> header in
----------------
ldionne wrote:
> I don't understand this comment, can you explain? In particular, what is a customization variable?
That's accidentally copy-pasted from `enable_borrowed_range.h`. These comment lines can just vanish.


================
Comment at: libcxx/include/__ranges/copyable_box.h:52
+
+    constexpr __copyable_box& operator=(const __copyable_box& __other)
+    noexcept(is_nothrow_copy_constructible_v<_Tp>)
----------------
cjdb wrote:
> ldionne wrote:
> > Maybe we should add `requires !copyable<_Tp>` here, and `requires !movable<_Tp>` below to make it clearer?
> That partially defeats the point of concepts, which is to not need doing that.
I put my thoughts on this question here:
https://quuxplusone.github.io/blog/2021/06/07/tag-dispatch-and-concept-overloading/#it-seems-to-be-the-current-wisdo
If the overloads were complicated or separated by dozens of lines of code, I'd think Louis' point is reasonable. As it is, I'm neutral.


================
Comment at: libcxx/include/module.modulemap:560
+      module concepts               { header "__ranges/concepts.h" }
+      module copyable_box           { header "__ranges/copyable_box.h" export optional }
+      module data                   { header "__ranges/data.h" }
----------------
Incidentally, another side benefit of the planned rewrite to compose-not-inherit from `optional` is that whatever this little `export optional` hack is, will no longer be necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102135