[libcxx-commits] [PATCH] D116023: [libc++][ranges] Implement `uninitialized_copy{, _n}` and `uninitialized_move{, _n}`.

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:182
 inline namespace __cpo {
 inline constexpr auto uninitialized_fill = __uninitialized_fill::__fn(__function_like::__tag());
 } // namespace __cpo
----------------
Does this just need to be rebased? I think I remember fixing that `fil` to `fill` as a drive-by, too.
Ditto throughout: I believe we agreed (at least for now) not to indent the big namespace but to leave the one-liner indented. Anyway, please look at the existing code (as it is now in `main`, not how it was last month ;)) and let's be consistent with whatever it looks like.


================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:237
+
+  // clang-format off
+  template <input_range _InputRange, __nothrow_forward_range _OutputRange>
----------------
Please remove all `clang-format` comments.


================
Comment at: libcxx/include/__memory/ranges_uninitialized_algorithms.h:271-272
+  uninitialized_copy_n_result<_InputIterator, _OutputIterator>
+  operator()(_InputIterator __ifirst, iter_difference_t<_InputIterator> __n, _OutputIterator __ofirst,
+             _Sentinel __olast) const {
+    // clang-format on
----------------
Readability nit. ("Semantic linebreaks")


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:30
+  template <class _Iter>
+  _LIBCPP_HIDE_FROM_ABI friend _LIBCPP_CONSTEXPR bool operator!=(const _Iter&, __unreachable_sentinel) _NOEXCEPT {
+    return true;
----------------
I suspect the `_NOEXCEPT` is useless and thus should be removed; but I'm not sure. (Which is why I'd rather it hadn't been there at all! [Frost's corollary to Chesterton's Fence](https://www.youtube.com/watch?v=OQgFEkgKx2s&t=780s): it's going to take more work to remove a fence [keyword] than to erect one unnecessarily, so let's make sure we're only erecting the necessary ones.)


================
Comment at: libcxx/include/__memory/uninitialized_algorithms.h:37-39
+template <class _ValueType, class _InputIterator, class _Sentinel1, class _ForwardIterator, class _Sentinel2>
+inline _LIBCPP_HIDE_FROM_ABI pair<_InputIterator, _ForwardIterator>
+__uninitialized_copy(_InputIterator __ifirst, _Sentinel1 __ilast, _ForwardIterator __ofirst, _Sentinel2 __olast) {
----------------
and so on throughout — let's not go out of our way to fit everything in 80 columns, but let's not go out of our way to shove everything onto one line either :)


================
Comment at: libcxx/test/std/algorithms/algorithms.results/in_out_result.pass.cpp:23-30
+#ifndef _LIBCPP_ABI_MICROSOFT // [[no_unique_address]] currently isn't implemented on Windows.
+// Size optimization.
+struct Empty {};
+struct Empty2 {};
+static_assert(sizeof(std::ranges::in_out_result<int, Empty>) == sizeof(int));
+static_assert(sizeof(std::ranges::in_out_result<Empty, int>) == sizeof(int));
+static_assert(sizeof(std::ranges::in_out_result<Empty, Empty2>) == sizeof(char));
----------------
I remember a similar issue in the `in_in_result` review. Is this consistent with what we landed on there? (I don't //think// it is...?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116023