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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

At first, I didn't understand the purpose of this type. Now I think I do - it's to turn something that is copy-constructible (but maybe not copy assignable) into something that is both copy-constructible and copy-assignable. It does that by using an optional and basically doing destroy-then-copy-construct in the assignment operator.

However, we are missing the optimization mentioned at http://eel.is/c++draft/range.adaptors#range.copy.wrap-2. Basically, if we know that `T` is `copyable` or `is_nothrow_move_constructible && is_nothrow_copy_constructible`, we can forego the `bool engaged` bit of information and store a straight `T`. Indeed, if the type is `copyable`, then the `operator=` is trivial to implement, by using the `operator=` provided by `T` itself. Similarly, if the type is `nothrow_copy_constructible`, we know we can do the destroy-and-then-construct dance without the copy construction throwing an exception, and hence we're able to provide the correct exception guarantee without introducing an empty state.

We can implement it roughly as:

  template<class _Tp>
    requires copyable<_Tp> || (is_nothrow_move_constructible_v<_Tp> && is_nothrow_copy_constructible_v<_Tp>)
  struct __copyable_box<_Tp> {
    __copyable_box& operator=(__copyable_box const& __other) noexcept(to-be-figured-out) {
      if constexpr (copyable<_Tp>) {
        __val_ = __other.__val_;
      } else { // is_nothrow_xxx is satisfied
        _VSTD::destroy_at(&__val_);
        _VSTD::construct_at(&__val_, other.__val_);
      }
      return *this;
    }
  
    // The rest follows naturally
  private:
    [[no_unique_address]] _Tp __val_; // no-unique-address not mandatory, but why not!
  };

WDYT?



================
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
----------------
I don't understand this comment, can you explain? In particular, what is a customization variable?


================
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> {
----------------
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?


================
Comment at: libcxx/include/__ranges/copyable_box.h:38
+  requires is_object_v<_Tp>
+  class __copyable_box : public optional<_Tp> {
+  public:
----------------
Please make this a member. Let's expose the smallest API that it makes sense to expose, it eases maintenance and understanding of how the API is used.

When I read http://eel.is/c++draft/range.adaptors#range.copy.wrap-1:

> `copyable-box<T>` behaves exactly like `optional<T>` with the following differences

I don't read this as meaning "it has the same API as `optional<T>`". I understand it as being an internal type with whatever API is needed to fulfill the implementation, but with a behavior equivalent to `optional<T>` for that API (except where specified otherwise). MSVC also doesn't seem to think that the whole `optional` API is needed: https://github.com/microsoft/STL/blob/master/stl/inc/ranges#L247.

Also, not all standard library types are written with the use case of being inherited from in mind. It does work, sure, but it may end up being somewhat weird. For example, when you say

```
using optional<_Tp>::optional;
```

below, you end up bringing in all the constructors like this one:

```
template < class U >
optional( const optional<U>& other );
```

It certainly feels weird to me that the following line would compile:

```
__copyable_box<T> box = std::make_optional(t);
```



================
Comment at: libcxx/include/__ranges/copyable_box.h:52
+
+    constexpr __copyable_box& operator=(const __copyable_box& __other)
+    noexcept(is_nothrow_copy_constructible_v<_Tp>)
----------------
Maybe we should add `requires !copyable<_Tp>` here, and `requires !movable<_Tp>` below to make it clearer?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102135