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

Thu Jul 1 09:28:55 PDT 2021

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

================
Comment at: libcxx/include/__ranges/copyable_box.h:89
+
+    constexpr _Tp const& operator*() const { return *__val_; }
+    constexpr _Tp& operator*() { return *__val_; }
----------------
Quuxplusone wrote:
> zoecarver wrote:
> > This and the other's need to be noexcept. (After my patch, std::optional's star will also be noexcept.)
> Since this is an internal helper, it doesn't //need// to be noexcept; there's definitely nothing in libc++ (and by definition, nothing outside libc++) that would ever check its noexceptness-status.
> We know from other reviews that there is a danger in accidentally saying `noexcept` when the function might throw (namely, rogue calls to `std::terminate`), whereas there is no danger in accidentally saying nothing when the function doesn't throw.
> However, I wouldn't strongly //object// to marking it noexcept, //because// it's so trivial that its noexceptness is "obvious" (famous last words).
@zoecarver  I'm not sure I understand why it needs to be `noexcept`. Can you please explain?

================
Comment at: libcxx/include/__ranges/copyable_box.h:124
+    __copyable_box& operator=(__copyable_box const&) requires copyable<_Tp> = default;
+    __copyable_box& operator=(__copyable_box&&) requires copyable<_Tp> = default;
+
----------------
CaseyCarter wrote:
> Why not `requires movable<_Tp>`? (Don't forget to update the redundant constraint on line 135.) This enables you to relax the requirements on this specialization to `copyable<_Tp> || (is_nothrow_copy_constructible_v<_Tp> && (movable<_Tp> || is_nothrow_move_constructible_v<_Tp>))`. Not that I think there's much to be gained thereby - it's hard to envision a nothrow-copy_constructible type that isn't also nothrow-move_constructible.
Good point. The reason why I wrote it that way is that I originally had two specializations, one for `copyable` and one for `is_nothrow_xxx_constructible`. I then merged the two implementations somewhat mechanically.

However, I gave more thought and here's what I'm doing now:

```
template<class _Tp>
concept __doesnt_need_empty_state_for_copy = copyable<_Tp> || is_nothrow_copy_constructible_v<_Tp>;

template<class _Tp>
concept __doesnt_need_empty_state_for_move = movable<_Tp> || is_nothrow_move_constructible_v<_Tp>;

template<__copy_constructible_object _Tp>
  requires __doesnt_need_empty_state_for_copy<_Tp> && __doesnt_need_empty_state_for_move<_Tp>
class __copyable_box<_Tp> {
  // Implementation of assignment operators in case we perform optimization (1)
  constexpr __copyable_box& operator=(__copyable_box const&) requires copyable<_Tp> = default;
  constexpr __copyable_box& operator=(__copyable_box&&) requires movable<_Tp> = default;

  // Implementation of assignment operators in case we perform optimization (2)
  constexpr __copyable_box& operator=(__copyable_box const& __other) noexcept { ... }
  constexpr __copyable_box& operator=(__copyable_box&& __other) noexcept { ... }
};
```

It's somewhat of a mouthful, but I feel like it expresses what we're trying to do most clearly.

================
Comment at: libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/assign.copy.pass.cpp:44
+      Box x(std::in_place, 5);
+      Box& result = (x = x);
+
----------------
Quuxplusone wrote:
> Should we test that this (does | doesn't) end up calling `CopyConstructible::operator=`?
Yes, I think we should. Note that we don't need to test that we don't call `operator=` in the cases that we don't, since `operator=` is marked as deleted in those cases.

================
Comment at: libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/operator-bool.pass.cpp:24-25
+  std::ranges::__copyable_box<T> const x(std::in_place, 10);
+  bool result = static_cast<bool>(x);
+  assert(result);
+
----------------
Quuxplusone wrote:
> Perhaps also test
> ```
> static_assert(!std::is_convertible_v<decltype(x), bool>);
> ```
> However, I have a higher-level question: Why on earth do we want this //internal implementation detail type// to be contextually convertible to bool in the first place?!  It should just have a `.has_value()` member, and we should use `x.has_value()` everywhere we're testing whether it has a value. I don't see any situation where we'd be like "ooh, I wish I could just omit the `.has_value` part of this expression and it would still compile and do the same thing." For our internal detail types, we have the luxury of getting to design their APIs ourselves, and we don't need to build in that kind of footgun if we don't want to.
Hmm, I guess I agree. I added `__has_value()` and removed `operator bool`.

================
Comment at: libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/properties.compile.pass.cpp:37
+// primary template
+static_assert(sizeof(std::ranges::__copyable_box<CopyConstructible>) > sizeof(CopyConstructible)); // must have an empty state
+
----------------
zoecarver wrote:
> I might argue that we don't really need this line. But I suppose it doesn't really hurt. 
> 
> (The reason being, we don't really care if these are equal in size, we care if the throwing behavior works.)
Actually, I think we do care about pinning this down for ABI purposes. If our implementation changed somehow, we'd want to make sure that we don't break the ABI, and this is one step towards that. I'll strengthen this to `sizeof(copyable-box) == sizeof(std::optional<T>)`.

================
Comment at: libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/types.h:13
+#include <cassert>
+#include <__ranges/copyable_box.h>
+
----------------
Quuxplusone wrote:
> I'd rather see just `#include <ranges>` here.
Can you explain why? Does that apply to the other test files too?

I like including `<__ranges/copyable_box.h>` since it's an implementation-detail type, and it has the added benefit that it tests that the header can be included on its own.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102135