[libcxx-commits] [PATCH] D151629: [libc++][ranges] Implement P2494R2 (Relaxing range adaptors to allow for move only types)

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.

Thanks for the patch!



================
Comment at: libcxx/include/__ranges/filter_view.h:56
     _LIBCPP_NO_UNIQUE_ADDRESS _View __base_ = _View();
-    _LIBCPP_NO_UNIQUE_ADDRESS __copyable_box<_Pred> __pred_;
+    _LIBCPP_NO_UNIQUE_ADDRESS __movable_box<_Pred> __pred_;
 
----------------
For each of these changes to existing views, we need a new test(s) that would fail if not for the change in this patch.


================
Comment at: libcxx/include/__ranges/movable_box.h:10
+
+#ifndef _LIBCPP___RANGES_MOVABLE_BOX_H
+#define _LIBCPP___RANGES_MOVABLE_BOX_H
----------------
The formatting changes make it harder to see things that are relevant to the proposal in this file. Can you clang-format `copyable_box.h` in a separate patch and rebase this patch on that?


================
Comment at: libcxx/include/__ranges/movable_box.h:33
+
+// __movable_box allows turning a type that is copy-constructible (but maybe not copy-assignable) into
+// a type that is both copy-constructible and copy-assignable. It does that by introducing an empty state
----------------
Is this comment up-to-date?


================
Comment at: libcxx/include/__ranges/movable_box.h:44
+concept __movable_box_object =
+#if _LIBCPP_STD_VER >= 23
+  move_constructible<_Tp>
----------------
Is there a reason not to apply the new behavior to old language versions retroactively? Would this change break any existing code?

(Also tagging @ldionne )


================
Comment at: libcxx/include/__ranges/single_view.h:40
+public:
+  _LIBCPP_HIDE_FROM_ABI single_view()
+    requires default_initializable<_Tp>
----------------
Can you please undo the formatting change? It makes it harder to see the parts of the diff that implement the proposal.


================
Comment at: libcxx/test/libcxx/ranges/range.adaptors/range.move.wrap/arrow.pass.cpp:11
 
 // T* <copyable-box>::operator->()
 // const T* <copyable-box>::operator->() const
----------------
`s/copyable-box/movable-box/`. Applies throughout.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151629