[libcxx-commits] [PATCH] D142335: [libc++][ranges] Implement `ranges::to`.

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__iterator/ranges_iterator_traits.h:28
+template <ranges::input_range _Range>
+using __range_key_type = __remove_const_t<typename ranges::range_value_t<_Range>::first_type>;
+
----------------



================
Comment at: libcxx/include/__ranges/from_range.h:24
+struct from_range_t {
+  _LIBCPP_HIDE_FROM_ABI explicit from_range_t() = default;
+};
----------------
I think with the current HIDE_FROM_ABI rules this shouldn't be marked.


================
Comment at: libcxx/include/__ranges/to.h:47
+
+// Note: in the Standard, `reservable-container` is a `constexpr bool` variable. However, GCC has a compiler bug that
+// makes short-circuiting not work properly in that case, so use a concept as a workaround.
----------------
Do you know whether GCC 13 fixes that bug? If yes, maybe make this a TODO, since we will switch very soon?


================
Comment at: libcxx/include/__ranges/to.h:90-93
+  static_assert(same_as<_Container, remove_const_t<_Container>>,
+                "The target container cannot be const-qualified, please remove the const");
+  static_assert(same_as<_Container, remove_volatile_t<_Container>>,
+                "The target container cannot be volatile-qualified, please remove the volatile");
----------------
Why not `is_const` and `is_volatile`?


================
Comment at: libcxx/include/__ranges/to.h:127
+    } else {
+      static_assert(__always_false<_Container>, "ranges::to: unable to convert to the given container type.");
+    }
----------------
Is this the only use for `__always_false`? If yes, good news: `static_assert(false)` gets accepted by clang 17 and GCC 13, so this can soon just be what it was always supposed to be.


================
Comment at: libcxx/include/__ranges/to.h:240
+_LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr auto to(_Args&&... __args) {
+  return __range_adaptor_closure_t(std::__bind_back(__to_deduced<_Container>(), std::forward<_Args>(__args)...));
+}
----------------



================
Comment at: libcxx/include/__ranges/to.h:161
+        _Container(std::declval<_Range>(), std::declval<_Args>()...));
+      return (_Result*){};
+
----------------
ldionne wrote:
> 
I think adding and removing a pointer is actually more efficient thann using `type_identity`, since adding a pointer and removing it avoids instantiating a new type every time.


================
Comment at: libcxx/include/deque:792
+    void prepend_range(_Range&& __range) {
+      insert(begin(), std::forward<_Range>(__range));
+    }
----------------
var-const wrote:
> philnik wrote:
> > This looks completely wrong. Did you mean to use `insert_range`? I'm also not convinced this can be implemented this way. `prepend_range` has much stronger exception guarantees than `insert`. If we provide strong enough exception guarantees inside `insert` (which I don't think is the case), please add a comment.
> Yes, it should be `insert_range`.
> 
> Where are the exception guarantees you're referring to specified? I was looking at https://eel.is/c++draft/deque.modifiers where `insert`, `insert_range` and `prepend_range` are grouped together and share the same `remarks` about exception safety.
IDK. I must have been misremembering something wrong.


================
Comment at: libcxx/include/map:66
     explicit map(const key_compare& comp);
     map(const key_compare& comp, const allocator_type& a);
     template <class InputIterator>
----------------
Note to self: continue here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142335