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

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

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__ranges/copyable_box.h:51
+  class __copyable_box {
+    std::optional<_Tp> __val_;
+
----------------
zoecarver wrote:
> zoecarver wrote:
> > `std::`
> Just curious: why is having a member better than inheriting? 
https://en.wikipedia.org/wiki/Composition_over_inheritance
https://quuxplusone.github.io/blog/2018/12/11/dont-inherit-from-std-types/#standard-types-are-like-boxes-of


================
Comment at: libcxx/include/__ranges/copyable_box.h:56
+      requires is_constructible_v<_Tp, _Args...>
+    constexpr explicit __copyable_box(in_place_t, _Args&& ...__args)
+      noexcept(is_nothrow_constructible_v<_Tp, _Args...>)
----------------
ldionne wrote:
> Oops, I just realized I'm missing a test for this constructor.
I'm skeptical of the use of a user-visible type like `in_place_t` here, for basically the same reason I'm skeptical of the `operator bool`. However, in this case I have no particular better suggestion.


================
Comment at: libcxx/include/__ranges/copyable_box.h:63
+      requires default_initializable<_Tp>
+      : __val_{in_place} // braces intentional
+    { }
----------------
This comment is either wrong (I hope) or horrible. If it's true that parens wouldn't work here... why not? //That's// what the comment should be explaining.


================
Comment at: libcxx/include/__ranges/copyable_box.h:91
+    constexpr _Tp& operator*() { return *__val_; }
+    constexpr explicit operator bool() const noexcept { return static_cast<bool>(__val_); }
+  };
----------------
I'd rather see `return __val_.has_value();` — it's shorter and also easier to understand what it's doing.


================
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);
+
----------------
Should we test that this (does | doesn't) end up calling `CopyConstructible::operator=`?


================
Comment at: libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/assign.copy.pass.cpp:113-114
+
+  // Tests for the empty state. Those can't be constexpr, since they are only reached
+  // through throwing an exception.
+  {
----------------
It would still be nice to move this chunk of tests into a helper function, e.g. `void test_empty_state()`.


================
Comment at: libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/assign.move.pass.cpp:22
+
+#pragma GCC diagnostic ignored "-Wself-move"
+
----------------
It'd be worth unifying the 2 different approaches to this diagnostic in our tests. Either way, don't add a 3rd.
```
test/std/input.output/filesystems/class.directory_iterator/directory_iterator.members/move_assign.pass.cpp:#pragma clang diagnostic ignored "-Wself-move"
test/std/input.output/filesystems/class.rec.dir.itr/rec.dir.itr.members/move_assign.pass.cpp:#pragma clang diagnostic ignored "-Wself-move"
test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.asgn/move.pass.cpp:// ADDITIONAL_COMPILE_FLAGS: -Wno-self-move
```


================
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);
+
----------------
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.


================
Comment at: libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/types.h:13
+#include <cassert>
+#include <__ranges/copyable_box.h>
+
----------------
I'd rather see just `#include <ranges>` here.


================
Comment at: libcxx/test/libcxx/ranges/range.adaptors/range.copy.wrap/types.h:41
+  NothrowCopyConstructible(NothrowCopyConstructible&&) noexcept = default;
+  NothrowCopyConstructible& operator=(NothrowCopyConstructible const&) = delete; // prevent from being copyable
+
----------------
It'd be useful to add `static_assert(!std::copyable<NothrowCopyConstructible>);` etc. — whatever you expect to be true of this type — right after its closing `};`. Ditto for the other types in this file. Ideally those static_asserts will help the reader see what the intended differences between these types are.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102135