[libcxx-commits] [PATCH] D115315: [libc++][ranges] Implement ranges::uninitialized_default_construct{, _n}.

Wed Dec 8 10:34:30 PST 2021

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

Other than `inline namespace __cpo` and some of my comments on the tests, this looks basically good to me.

================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:8
+//
+//===----------------------------------------------------------------------===//
+
----------------
I'm still weakly inclined to split this up into `ranges_uninitialized_default_construct.h`, `ranges_uninitialized_value_construct.h`, `ranges_uninitialized_copy.h`, `ranges_uninitialized_move.h`, `ranges_uninitialized_fill.h`.

I imagine it's OK to have `ranges::foo` and `ranges::foo_n` in the same header, since the Standard actually does the same thing with its subsection naming (e.g. both `ranges::uninitialized_fill` and `ranges::uninitialized_fill_n` are located in https://eel.is/c++draft/uninitialized.fill ). We have two bad options here: smushing all ten niebloids into 1 header of 175 lines, or spreading them out into 5 headers of 80 lines each.

I'm weakly in favor of "5 headers" but I guess "1 header of 175 lines total" doesn't sound too bad when I put it that way.

================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:32
+
+struct __uninitialized_default_construct_fn final : private __function_like {
+
----------------
I believe this `__function_like` business has caused weird Modules errors before (hence the workarounds you'll see in a few places in the modulemap). It's not needed for conformance, so I would strongly prefer you simply omit it — "Keep It Simple."
Likewise I would personally omit the `final` cruft (it disables the EBO but otherwise has no effect except taking longer to read), but it's not going to cause weird Modules errors, so I don't care as much about whether or not we add `final`. :)

================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:51
+
+};
+
----------------
Line 47: please `T&& t`, not `T &&t`
Lines 39, 46, 64: please indent the `requires`-clause two spaces

================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:53-54
+
+inline constexpr auto uninitialized_default_construct =
+    __uninitialized_default_construct_fn(__function_like::__tag());
+
----------------
Here and line 73, please `inline namespace __cpo { ~~~ }`. (See https://quuxplusone.github.io/blog/2021/12/07/namespace-cpo/ for my rationale, which boils down to "consistency.")

================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:68
+    using _ValueType = remove_reference_t<iter_reference_t<_ForwardIterator>>;
+    return _VSTD::__uninitialized_default_construct_n<_ValueType>(__first, __n);
+  }
----------------
This is actually specced as
```
return uninitialized_default_construct(counted_iterator(first, n), default_sentinel).base();
```
but is the difference observable? I can't think of a reason it would be, so I'm fine with this as you've written it.

================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:132
     for (; __idx != __last; ++__idx)
-        ::new ((void*)_VSTD::addressof(*__idx)) _Vt;
+        ::new ((void*)_VSTD::addressof(*__idx)) _ValueType;
 #ifndef _LIBCPP_NO_EXCEPTIONS
----------------
var-const wrote:
> Note: this cast is simpler than the [`voidify`](http://eel.is/c++draft/specialized.algorithms#general-4) function that the Standard mandates. Should I fix this, or is there a reason why a simple cast to `void*` is sufficient here?
I can't think of any reason a simple cast to `void*` would be //insufficient//. I'd leave it simple until-and-unless someone comes up with a positive reason and gives us a regression test case. Otherwise, you'll complexify the code and someone six months from now will ask why it can't be simpler.

================
Comment at: libcxx/include/memory:235
+  requires default_initializable<iter_value_t<ForwardIterator>>
+ ForwardIterator ranges::uninitialized_default_construct_n(ForwardIterator first, iter_difference_t<ForwardIterator> n); // Since C++20
+
----------------
Here and above: lowercase `// since C++20`

================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.construct.default/ranges_uninitialized_default_construct.pass.cpp:61-71
+  alignas(Counted) char buffer[sizeof(Counted) * 1] = {};
+  auto* as_array = reinterpret_cast<Counted*>(buffer);
+
+  ranges::uninitialized_default_construct(as_array, as_array);
+  assert(Counted::current_objects == 0);
+  assert(Counted::total_objects == 0);
+
----------------
Let's also do some non-common ranges. I suggest these three tests in place of lines 68–71:
```
std::ranges::uninitialized_default_construct(std::ranges::empty<Counted>);  // [basically the same as your 68–69, but gives nullptr for begin and end instead of as_array]
assert(Counted::current_objects == 0);
assert(Counted::total_objects == 0);

auto it = forward_iterator<Counted*>(as_array);
auto r = std::ranges::subrange(it, sentinel_wrapper<forward_iterator<Counted*>>(it));
std::ranges::uninitialized_default_construct(r.begin(), r.end());
assert(Counted::current_objects == 0);
assert(Counted::total_objects == 0);

std::ranges::uninitialized_default_construct(r);
assert(Counted::current_objects == 0);
assert(Counted::total_objects == 0);
```

Ditto throughout: please make sure to test non-common ranges, because that's really the major feature you're actually adding here (e.g. that's why you had to modify the non-ranges version of the algorithm).

================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.construct.default/ranges_uninitialized_default_construct.pass.cpp:103-116
+void test_already_initialized() {
+  int array[] = {42, 42, 42};
+
+  ranges::uninitialized_default_construct(std::begin(array), std::end(array));
+  assert(array[0] == 42);
+  assert(array[1] == 42);
+  assert(array[2] == 42);
----------------
I'm pretty sure this is UB. `array[0]` is trivially destructible, so it's cool that you're skipping its destructor before creating a new `int` object at that location; but then you're //reading the value of// that new `int` object, and you haven't actually given it a value (default-constructing a primitive type doesn't set its value). So line 107 is UB.
Just remove this whole function. Even if it weren't UB, I'm not clear on what it's trying to test.

(and yeah, I say the same thing about `test_value_initialized()` from `uninitialized_default_construct.pass.cpp`)

================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.construct.default/ranges_uninitialized_default_construct.pass.cpp:15-17
+// template <nothrow-forward-iterator ForwardIterator, nothrow-sentinel-for<ForwardIterator> Sentinel>
+//   requires default_initializable<iter_value_t<ForwardIterator>>
+// ForwardIterator ranges::uninitialized_default_construct(ForwardIterator first, Sentinel last);
----------------
var-const wrote:
> Is it necessary to list out all the constraints here, or would just `ForwardIterator ranges::uninitialized_default_construct(ForwardIterator first, Sentinel last)` (or even just `ranges::uninitialized_default_construct`) suffice?
Personally, I'd err on the side of "just cut and paste what the Standard's synopsis says," which I believe is exactly what you've done here.

However, if you had just written `// ranges::uninitialized_default_construct`, I personally wouldn't have bothered to complain about it, either. :)

================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.construct.default/special_function.compile.pass.cpp:24-55
+namespace found_by_adl {
+
+template <class I = int*>
+struct adl_forward_iterator : forward_iterator<I> {
+  // To satisfy `weakly_incrementable`, the increment operators must return the same type as the
+  // class.
+  adl_forward_iterator& operator++();
----------------
This seems like overkill. (Certainly `adl_forward_iterator` needn't be a template! And you see why inheriting from things-that-aren't-meant-for-inheritance is a bad idea.)
I recommend replacing this whole thing with two lines, which can go in their respective .pass.cpp files:
```
static_assert(std::is_class_v<decltype(std::ranges::uninitialized_default_construct)>);
static_assert(std::is_class_v<decltype(std::ranges::uninitialized_default_construct_n)>);
```
I recommend adding a bonus test case for their SFINAE, as well:
```
struct NotDefaultCtable { NotDefaultCtable(int); };
static_assert(!std::is_invocable_v<decltype(std::ranges::uninitialized_default_construct), NotDefaultCtable*, NotDefaultCtable*>);
static_assert(!std::is_invocable_v<decltype(std::ranges::uninitialized_default_construct_n), NotDefaultCtable*, int>);
```

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115315