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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.

LGTM with comments. Thanks, I think this is great!



================
Comment at: libcxx/include/__ranges/to.h:164
+    if constexpr (requires { _Container(std::declval<_Range>(), std::declval<_Args>()...); }) {
+      using _Result = decltype( //
+          _Container(std::declval<_Range>(), std::declval<_Args>()...));
----------------
IMO this would fit nicely on a single line. Same below. Or make sure they line up with the `if constexpr` condition.


================
Comment at: libcxx/include/__ranges/to.h:220-221
+      !is_volatile_v<_Container>, "The target container cannot be volatile-qualified, please remove the volatile");
+
+  return __range_adaptor_closure_t(std::__bind_back(__to_nondeduced<_Container>(), std::forward<_Args>(__args)...));
+}
----------------
Would it be reasonable to try using a lambda here? Something like

```
auto __to_func = []<input_range _Range, class... _Args>(_Range&& __range, _Args&&... __args) -> _Container 
 requires requires { ranges::to<_Container>(std::forward<_Range>(__range), std::forward<_Args>(__args)...) }
{ return ranges::to<_Container>(std::forward<_Range>(__range), std::forward<_Args>(__args)...); };

return __range_adaptor_closure_t(std::__bind_back(__to_func, std::forward<_Args>(__args)...));
```

It's more compact and a bit more localized. Your call.


================
Comment at: libcxx/include/__ranges/to.h:67-69
+template <class _Container, class _Range>
+concept __can_try_convert_without_recursion =  !input_range<_Container> ||
+    convertible_to<range_reference_t<_Range>, range_value_t<_Container>>;
----------------
var-const wrote:
> ldionne wrote:
> > Maybe `__does_not_require_recursive_conversion`? Or `__try_non_recursive_conversion`? Or maybe just inline that in the function below and use an inline `requires`?
> Went with `__try_non_recursive_conversion` (unless you feel strongly about inlining it).
Re-reading it, I think it would be a lot more readable to inline, especially since there's an `else if` with the `input_range<_Container>` constraint.


================
Comment at: libcxx/include/__ranges/to.h:161
+        _Container(std::declval<_Range>(), std::declval<_Args>()...));
+      return (_Result*){};
+
----------------
var-const wrote:
> philnik wrote:
> > 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.
> Tagging @ldionne 
IMO `type_identity` is a lot more explicit about what we're doing. Between instantiating `type_identity` and `remove_pointer_t`, I think the difference is negligible.


================
Comment at: libcxx/test/std/ranges/range.utility/range.utility.conv/container.h:1
+//===----------------------------------------------------------------------===//
+//
----------------
Did you `clang-format` this?


================
Comment at: libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp:13-14
+//   constexpr C to(R&& r, Args&&... args);     // Since C++23
+// template<template<class...> class C, input_range R, class... Args>
+//   constexpr auto to(R&& r, Args&&... args);  // Since C++23
+
----------------
I think this should be removed now.


================
Comment at: libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp:47
+  // Deliberately not defining `iterator_category` to make sure this class satisfies the `input_iterator` concept but
+  // would fail `derived_from<iterator_category, input_iterator_tag`.
+
----------------



================
Comment at: libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp:513
+
+constexpr void test_recursive() {
+  using C1 = Container<int, CtrChoice::DirectCtr>;
----------------
Ok I love this! I was looking for something like it.


================
Comment at: libcxx/test/std/ranges/range.utility/range.utility.conv/to_deduction.pass.cpp:12
+// trunk -- `clang-17` should be removed from the failing list once that version of Clang is in the Docker image.
+// XFAIL: clang-16, clang-17, apple-clang-15
+
----------------



================
Comment at: libcxx/test/std/ranges/range.utility/range.utility.conv/to_deduction.pass.cpp:14-15
+
+// template<class C, input_range R, class... Args> requires (!view<C>)
+//   constexpr C to(R&& r, Args&&... args);     // Since C++23
+// template<template<class...> class C, input_range R, class... Args>
----------------
I think this should be removed now.


================
Comment at: libcxx/test/std/ranges/range.utility/range.utility.conv/to_std_containers.pass.cpp:11-14
+// template<class C, input_range R, class... Args> requires (!view<C>)
+//   constexpr C to(R&& r, Args&&... args);     // Since C++23
+// template<template<class...> class C, input_range R, class... Args>
+//   constexpr auto to(R&& r, Args&&... args);  // Since C++23
----------------
Let's update this comment to explain that we are testing general interoperation between std containers. That's what we're doing right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142335