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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked 7 inline comments as done.
ldionne added inline comments.


================
Comment at: libcxx/include/__ranges/copyable_box.h:36-37
+namespace ranges {
+  template<copy_constructible _Tp>
+  requires is_object_v<_Tp>
+  class __copyable_box : public optional<_Tp> {
----------------
cjdb wrote:
> ldionne wrote:
> > Unless there is a reason to give special treatment to `copy_constructible` over `is_object_v`, I would write it as:
> > 
> > ```
> > template <class _Tp>
> > requires copy_constructible<_Tp> && is_object_v<_Tp>
> > ```
> > 
> > WDYT?
> That's not idiomatic use of concepts.
I'd be quite curious to hear what's the rationale here.

But regardless, this is going to change and become constrained by a helper concept (which helps with the partial specialization for the copy-assignable optimization), so this will become moot when I update the patch.


================
Comment at: libcxx/include/__ranges/copyable_box.h:52
+
+    constexpr __copyable_box& operator=(const __copyable_box& __other)
+    noexcept(is_nothrow_copy_constructible_v<_Tp>)
----------------
Quuxplusone wrote:
> 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.
> That partially defeats the point of concepts, which is to not need doing that.

Hmm, I guess I agree. This won't be needed anyways once the specializations are split out.


================
Comment at: libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/properties.compile.pass.cpp:30
+
+struct not_copy_constructible {
+  not_copy_constructible() = default;
----------------
zoecarver wrote:
> cjdb wrote:
> > zoecarver wrote:
> > > Can you rename this to `move_only` or something? 
> > The current name spells out exactly what's missing. `move_only` implies copy-assignment is false.
> Sorry I misread the name on the first go around. This is good with me.
I need to change the tests quite a bit because the type changed so much, so I don't think this will be relevant anymore when I update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102135