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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:126
+inline _LIBCPP_HIDE_FROM_ABI
+_ForwardIterator __uninitialized_default_construct(_ForwardIterator __first, _Sentinel __last) {
     auto __idx = __first;
----------------
This should be a backward-compatible change -- the C++17 public function still checks that the passed parameters have the same type.


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:126
+inline _LIBCPP_HIDE_FROM_ABI
+_ForwardIterator __uninitialized_default_construct(_ForwardIterator __first, _Sentinel __last) {
     auto __idx = __first;
----------------
var-const wrote:
> This should be a backward-compatible change -- the C++17 public function still checks that the passed parameters have the same type.
This function now always returns the iterator to accommodate all callers. I presume this can be easily optimized away for the callers that don't use the result.


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


================
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);
----------------
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?


================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.construct.default/ranges_uninitialized_default_construct.pass.cpp:147
+
+int main() {
+  test_empty();
----------------
These tests are heavily inspired by `libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.construct.default/uninitialized_default_construct.pass.cpp`.


================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.construct.default/ranges_uninitialized_default_construct_n.pass.cpp:3
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
----------------
I think it would make sense to merge this file with the version taking iterator+sentinel / range because the tests are very similar and it would help cut down the boilerplate.


================
Comment at: libcxx/test/std/utilities/memory/specialized.algorithms/uninitialized.construct.default/uninitialized_default_construct.pass.cpp:26
   static int constructed;
-  static void reset() { count = constructed =  0; }
   explicit Counted() { ++count; ++constructed; }
----------------
It was unused.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115315