[libcxx-commits] [PATCH] D142335: [libc++][ranges] Implement `ranges::to`.
Konstantin Varlamov via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Mar 28 11:58:09 PDT 2023
var-const created this revision.
Herald added a subscriber: arichardson.
Herald added a project: All.
var-const updated this revision to Diff 491670.
var-const added a comment.
var-const updated this revision to Diff 491859.
var-const updated this revision to Diff 501774.
var-const marked 9 inline comments as done.
var-const updated this revision to Diff 503872.
var-const updated this revision to Diff 505752.
Herald added a subscriber: mikhail.ramalho.
var-const updated this revision to Diff 505896.
var-const updated this revision to Diff 507270.
var-const updated this revision to Diff 507651.
var-const retitled this revision from "DRAFT [libc++][ranges] Implement `ranges::to`." to "[libc++][ranges] Implement `ranges::to`.".
var-const published this revision for review.
Herald added a project: libc++.
Herald added a subscriber: libcxx-commits.
Herald added a reviewer: libc++.
Implement the `from_range_t` constructor in vector properly.
var-const added a comment.
Fix debug-only stuff
var-const added a comment.
Partially address feedback, add more tests, fix `deduce-expr`.
var-const added a comment.
from_range_t constructors and testing WIP
var-const added a comment.
Add from_range_t constructors to all containers except sets and maps.
var-const added a comment.
Add the `from_range_t` constructors to `map`.
var-const added a comment.
Finish adding `from_range_t` constructors to containers.
var-const added a comment.
Testing WIP.
var-const added a comment.
The tests are still WIP.
================
Comment at: libcxx/docs/Status/Cxx2bPapers.csv:45
+"`P1206R7 <https://wg21.link/P1206R7>`__","LWG","``ranges::to``: A function to convert any range to a container","February 2022","|Complete|","16.0","|ranges|"
"`P1413R3 <https://wg21.link/P1413R3>`__","LWG","Deprecate ``std::aligned_storage`` and ``std::aligned_union``","February 2022","",""
"`P2255R2 <https://wg21.link/P2255R2>`__","LWG","A type trait to detect reference binding to temporary","February 2022","",""
----------------
Yesterday we voted https://cplusplus.github.io/LWG/issue3847 in. So maybe it would be good to directly implement the resolution for that fix in this patch.
================
Comment at: libcxx/docs/Status/Cxx2bPapers.csv:45
+"`P1206R7 <https://wg21.link/P1206R7>`__","LWG","``ranges::to``: A function to convert any range to a container","February 2022","|Complete|","16.0","|ranges|"
"`P1413R3 <https://wg21.link/P1413R3>`__","LWG","Deprecate ``std::aligned_storage`` and ``std::aligned_union``","February 2022","",""
"`P2255R2 <https://wg21.link/P2255R2>`__","LWG","A type trait to detect reference binding to temporary","February 2022","",""
----------------
Mordante wrote:
> Yesterday we voted https://cplusplus.github.io/LWG/issue3847 in. So maybe it would be good to directly implement the resolution for that fix in this patch.
Thanks for the heads-up! Done.
================
Comment at: libcxx/docs/Status/Cxx2bPapers.csv:44
"`P0627R6 <https://wg21.link/P0627R6>`__","LWG","Function to mark unreachable code","February 2022","|Complete|","15.0"
-"`P1206R7 <https://wg21.link/P1206R7>`__","LWG","``ranges::to``: A function to convert any range to a container","February 2022","","","|ranges|"
+"`P1206R7 <https://wg21.link/P1206R7>`__","LWG","``ranges::to``: A function to convert any range to a container","February 2022","|Complete|","16.0","|ranges|"
"`P1413R3 <https://wg21.link/P1413R3>`__","LWG","Deprecate ``std::aligned_storage`` and ``std::aligned_union``","February 2022","",""
----------------
I think this will miss the deadline for LLVM 16, and it'll be too large to cherry-pick, so I'd suggest aiming it for LLVM 17.
================
Comment at: libcxx/include/__memory/container_compatible_range.h:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
I think I would suggest moving this to `__ranges`, or even to `__concepts`.
================
Comment at: libcxx/include/__memory/container_compatible_range.h:28-29
+ ranges::input_range<_Range> && convertible_to<ranges::range_reference_t<_Range>, _Tp>;
+
+
+#endif // _LIBCPP_STD_VER >= 23
----------------
================
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>>;
----------------
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`?
================
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>>;
----------------
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).
================
Comment at: libcxx/include/__ranges/to.h:97
+ common_range<_Range> &&
+ __iterator_traits_detail::__cpp17_input_iterator<iterator_t<_Range>> &&
+ constructible_from<_Container, iterator_t<_Range>, sentinel_t<_Range>, _Args...>
----------------
Can you double-check that this is the right concept? And we should also move it outside of that detail namespace -- that's an unusual way to implement an exposition-only concept, but this comes from the very beginning of our ranges implementation. Basically let's clean up all those concepts in `__iterator_traits_detail`.
================
Comment at: libcxx/include/__ranges/to.h:98
+ __iterator_traits_detail::__cpp17_input_iterator<iterator_t<_Range>> &&
+ constructible_from<_Container, iterator_t<_Range>, sentinel_t<_Range>, _Args...>
+ ) {
----------------
Can you add tests that would fail if this didn't short-circuit properly? TBH I think those tests would fail right now.
================
Comment at: libcxx/include/__ranges/to.h:123-124
+ } else if constexpr (input_range<range_reference_t<_Range>>) {
+ return to<_Container>(__range | views::transform([](auto&& __elem) {
+ return to<range_value_t<_Container>>(std::forward<decltype(__elem)>(__elem));
+ }), std::forward<_Args>(__args)...);
----------------
I think those should be qualified calls.
================
Comment at: libcxx/include/__ranges/to.h:161
+ _Container(std::declval<_Range>(), std::declval<_Args>()...));
+ return (_Result*){};
+
----------------
================
Comment at: libcxx/include/__ranges/to.h:185
+
+ using type = remove_pointer_t<decltype(__deduce_func())>;
+
----------------
================
Comment at: libcxx/include/__ranges/to.h:53
+ requires (requires { __c.push_back(std::forward<_Ref>(__ref)); } ||
+ // Insufficient constraint: also need to check that inserter(...) is well-formed (the type e.g. must have
+ // begin() defined, have value_type typedef, etc.).
----------------
I think there's no better way of doing it except for checking that all the things that `std::insert_iterator` would use in its implementation are well-formed. I suspect the standard is aware of that and they just decided not to go down that route.
================
Comment at: libcxx/include/__ranges/to.h:72
+template <class>
+concept __false = false;
+
----------------
I wouldn't be surprised if some system headers somewhere already defined `__false`, so I think we might want to steer clear of that.
We may actually already have something like that using a constexpr bool variable.
================
Comment at: libcxx/include/__ranges/to.h:72
+template <class>
+concept __false = false;
+
----------------
ldionne wrote:
> I wouldn't be surprised if some system headers somewhere already defined `__false`, so I think we might want to steer clear of that.
>
> We may actually already have something like that using a constexpr bool variable.
I thought so too but couldn't find it.
================
Comment at: libcxx/include/__ranges/to.h:196
+ using _DeduceExpr = typename _Deducer<_Container, _Range, _Args...>::type;
+ return to<_DeduceExpr>(std::forward<_Range>(__range), std::forward<_Args>(__args)...);
+}
----------------
Did you intend to have ADL here?
================
Comment at: libcxx/include/deque:609
+ template <_ContainerCompatibleRange<_Tp> _Range>
+ _LIBCPP_HIDE_FROM_ABI deque(from_range_t, _Range&& __range,
+ const allocator_type& __a = allocator_type())
----------------
Testing: We should make sure that we do the right thing if we throw halfway through the construction. Do we destroy the elements constructed so far? Do we do that in reverse construction order?
I think the answer right now is "no" to both of these questions. If so, and if that's a pre-existing issue for other constructors of `deque`, and if the standard requires us to do that, let's at least file a bug report so we can remember to fix that.
================
Comment at: libcxx/include/deque:604
+ : __map_(__pointer_allocator(__a)), __start_(0), __size_(0, __a) {
+ if constexpr (ranges::sized_range<_Range>) {
+ __append_n(ranges::begin(__range), ranges::size(__range));
----------------
In the case of `deque`, it's not clear to me that we gain much from using `__append_n`. In both cases, we end up performing `N / CHUNK_SIZE` allocations and then copying the whole input range into these allocations. Maybe worth measuring? I think looking at how `__add_back_capacity(size_type __n)` and `__add_back_capacity()` differ will provide some additional insights here.
Also, if we end up keeping this optimization, the `if constexpr` condition seems a bit off. What should we do for a `forward_range` that is not a `sized_range`? We'll have to determine whether we want to take the hit to compute `distance(rng)` and then call `__append_n`, or not.
================
Comment at: libcxx/include/list:899-906
+ template <_ContainerCompatibleRange<_Tp> _Range>
+ _LIBCPP_HIDE_FROM_ABI list(from_range_t, _Range&& __range,
+ const allocator_type& __a = allocator_type()) : base(__a) {
+ std::__debug_db_insert_c(this);
+ for (auto&& __e : __range) {
+ __emplace_back(std::forward<decltype(__e)>(__e));
+ }
----------------
Does the Standard specify whether we should be forwarding elements from the input range into the container being constructed? For example, let's say we have this:
```
std::vector<std::string> v{"hello", "world", "something long"};
std::list<std::string> list(std::from_range, std::move(v));
```
We have an expiring `vector<string>` here and we could technically move the strings from it and into the `std::list` being constructed, however is that something we're allowed to do?
This probably applies elsewhere too.
Edit: I don't think we are allowed to do that, cause no standard API would allow us to achieve this. We'd need to use a `move_iterator` on the input `vector<string>` to get that effect, and there's really no way to do that naturally (e.g. `std::forward(v).begin()` won't give you back a `move_iterator`). So I think your code is fine as-is.
================
Comment at: libcxx/include/list:904
+ for (auto&& __e : __range) {
+ __emplace_back(std::forward<decltype(__e)>(__e));
+ }
----------------
I think this actually raises another interesting question. What happens if you do:
```
std::vector<std::string> v{"hello", "world", "something long"};
std::ranges::subrange r(std::move_iterator(v.begin()), std::move_iterator(v.end()));
std::list<std::string> list(std::from_range, r);
```
Right now, this will do the right thing and it will move elements out of the `subrange` (aka the `vector`'s elements), since you use `std::forward`. But if you didn't use `std::forward`, it wouldn't move the strings out. For `std::string` that's only an optimization, but if you have a move-only type, then that becomes a matter of conformance, I think. I think the Standard probably implicitly mandates that this needs to work. So I would test with something like:
```
struct MoveOnly { ... };
MoveOnly data[...] = {...};
std::ranges::subrange subrange(std::move_iterator(data.begin()), std::move_iterator(data.end()));
std::list<MoveOnly> list(std::from_range, subrange);
```
(pseudo-code)
================
Comment at: libcxx/include/list:1177
+
+template <ranges::input_range _Range, class _Allocator = allocator<ranges::range_value_t<_Range>>>
+list(from_range_t, _Range&&, _Allocator = _Allocator())
----------------
Please mark as done once you have tests for these CTAD guides (for all containers).
================
Comment at: libcxx/include/map:1099-1101
+#if _LIBCPP_STD_VER >= 23
+
+ template <_ContainerCompatibleRange<value_type> _Range>
----------------
Nitpick but I would not put a blank line between those, it seems a bit superfluous and in this case a bit inconsistent with surrounding code. But I don't care strongly.
================
Comment at: libcxx/include/map:1330
+ _LIBCPP_HIDE_FROM_ABI
+ void insert_range(from_range_t, _Range&& __range) {
+ const_iterator __end = cend();
----------------
This should also have tests for the MoveOnly behavior.
================
Comment at: libcxx/include/queue:478
+ class _Alloc,
+ class = __enable_if_t<__is_allocator<_Alloc>::value>>
+ queue(from_range_t, _Range&&, _Alloc)
----------------
Not sure if this SFINAE is actually needed (I followed the existing pattern). Concepts and the `from_range_t` should work to distinguish these from existing CTAD "overloads", and there is no ambiguity between the 2 and 3 argument versions.
================
Comment at: libcxx/include/ranges:353
+
+ struct from_range_t { explicit from_range_t() = default; }; // Since C++23
+ inline constexpr from_range_t from_range{}; // Since C++23
----------------
This is nitpicky but let's make sure we have a test that the constructor is explicit.
================
Comment at: libcxx/include/string:1932-1933
+ void __init_with_size(_InputIterator __first, _Sentinel __last, size_type __sz) {
+ if (__libcpp_is_constant_evaluated())
+ __r_.first() = __rep();
+
----------------
Maybe call `__default_init()` here unconditionally for consistency with above?
================
Comment at: libcxx/include/vector:2425
+ _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20
+ void __construct_at_end(_ForwardIterator __first, _Sentinel __last, size_type __n);
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __append(size_type __n, const_reference __x);
----------------
I think this part of the patch is also NFC and could easily be extracted, since you're mainly hoisting the `__n` computation to outside of `__construct_at_end`.
================
Comment at: libcxx/include/vector:727
- template <class _ForwardIterator, __enable_if_t<__is_cpp17_forward_iterator<_ForwardIterator>::value, int> = 0>
- _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void
- __construct_at_end(_ForwardIterator __first, _ForwardIterator __last, size_type __n);
+ template <class _ForwardIterator, class _Sentinel>
+ _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI
----------------
If that makes sense, it may be useful to make these changes in a separate patch prior to this patch. Just suggesting in case it makes things simpler.
================
Comment at: libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp:48
+static_assert(!std::ranges::view<ContainerT>);
+static_assert(HasTo<ContainerT, InputRange>);
+static_assert(!HasTo<test_view<forward_iterator>, InputRange>);
----------------
I think this is technically ill-formed according to the spec. I wonder whether it makes sense to test the exact way in which it is ill-formed as we do here (probably not).
Also note that if that's correct and this is ill-formed, then technically we could also be SFINAE friendly as a QOI matter. I don't think we should unless it's simple and other implementations are doing it as well.
================
Comment at: libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp:159-161
+ for (int i = Capacity - 2; i >= where_index; --i) {
+ buffer_[i + 1] = buffer_[i];
+ }
----------------
Can we use `std::move` or `std::move_backward` here?
================
Comment at: libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp:178
+
+ constexpr size_t max_size() const
+ requires CanReserve
----------------
Here and everywhere else -- this is going to become an error once we have C++20 modules.
================
Comment at: libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp:186
+constexpr void test_ctr_choice_order() {
+ std::array in = {1, 2, 3, 4, 5};
+ int arg1 = 42;
----------------
It would be nice to templatize the tests on the iterator category, and to run them with our various iterator archetypes.
================
Comment at: libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp:188
+ int arg1 = 42;
+ char arg2 = 'a';
+
----------------
Nothing in this test seems to rely on the contents of `in`, so I think it would also be nice to run it on a couple of different inputs:
- Empty sequence
- One element sequence
- Two elements
- etc
================
Comment at: libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp:197
+ assert(std::ranges::equal(result, in));
+ assert((in | std::ranges::to<C>()) == result);
+ }
----------------
Could we get one test that does something like:
```
auto closure = std::ranges::to<C>();
std::same_as<C> decltype(auto) result = in | closure;
assert(result == the-result-you-would-get-with-ranges::to-applied-directly);
```
No need to test it for all the constructor choices, but I think checking it once it useful.
================
Comment at: libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp:318
+struct NotARange {
+ constexpr NotARange(std::ranges::input_range auto&&)
+ requires (Rank >= CtrChoice::DirectCtr)
----------------
Are there other constructors that can be tested here?
================
Comment at: libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp:355
+
+ A4 in = {};
+ std::same_as<C4> decltype(auto) result = std::ranges::to<C4>(in);
----------------
It would be nice to check the contents too. To avoid this being too unwieldy, what about this?
```
int x = 0;
for (auto& a : in)
for (auto& b : a)
for (auto& c : b)
c = x++;
```
I'm happy with any way that makes this not unwieldy to check. One thing we could also do is check the content for a 2 or 3 dimensional array, not a 4 dimensional array.
================
Comment at: libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp:484
+
+ ////////////////////////////////////////////////////////////////////////////////
+ {
----------------
Reminder to remove
================
Comment at: libcxx/test/std/ranges/range.utility/range.utility.conv/to_std_containers.pass.cpp:53-55
+ std::set<typename T::value_type> a(lhs.begin(), lhs.end());
+ std::set<typename U::value_type> b(rhs.begin(), rhs.end());
+ return std::ranges::equal(a, b);
----------------
I think `is_permutation` might be a good candidate to use here.
================
Comment at: libcxx/test/std/ranges/range.utility/range.utility.conv/to_std_containers.pass.cpp:69-89
+
+template <class T>
+using all_containers = types::type_list<
+ std::vector<T>,
+ std::deque<T>,
+ std::list<T>,
+ std::forward_list<T>,
----------------
Here's a suggestion that would avoid having to define `is_equal` with a bit of magic:
```
template <class T>
using sequence_containers = types::type_list<std::vector<T>, std::deque<T>, std::list<T>, std::forward_list<T>>;
template <class T>
using set_like_associative_containers = types::type_list<std::set<T>, std::multiset<T>, std::unordered_set<T>, std::unordered_multiset<T>>;
template <class Key, class Value>
using map_like_associative_containers = types::type_list<std::map<Key, Value>, std::multimap<Key, Value>, std::unordered_map<Key, Value>, std::unordered_multimap<Key, Value>>;
void test() {
// Sequence = Sequence
types::for_each(sequence_containers<int>{}, []<class To>() {
types::for_each(sequence_containers<int>{}, []<class From>() {
test_conversion<From, To>(/* equal = */[](auto const& a, auto const& b) { return std::ranges::equal(a, b); });
});
});
// Set-like = Set-like
types::for_each(set_like_containers<int>{}, []<class To>() {
types::for_each(set_like_containers<int>{}, []<class From>() {
test_conversion<From, To>(/* equal = */[](auto const& a, auto const& b) { return std::ranges::is_permutation(a, b); });
});
});
// Map-like = Map-like
types::for_each(map_like_containers <int, int>{}, []<class To>() {
types::for_each(map_like_containers<int, int>{}, []<class From>() {
test_conversion<From, To>(/* equal = */[](auto const& a, auto const& b) { return std::ranges::is_permutation(a, b); });
});
});
// Sequence = Set-like and Set-like = Sequence
types::for_each(sequence_containers<int>{}, []<class To>() {
types::for_each(set_like_associative_containers<int>{}, []<class From>() {
test_conversion<From, To>(/* equal = */[](auto const& a, auto const& b) { return std::ranges::is_permutation(a, b); });
test_conversion<To, From>(/* equal = */[](auto const& a, auto const& b) { return std::ranges::is_permutation(a, b); });
});
});
// Sequence = Map-like and Map-like = Sequence
types::for_each(sequence_containers<std::pair<int, int>>{}, []<class To>() {
types::for_each(map_like_associative_containers<int, int>{}, []<class From>() {
test_conversion<From, To>(/* equal = */[](auto const& a, auto const& b) { return std::ranges::is_permutation(a, b); });
test_conversion<To, From>(/* equal = */[](auto const& a, auto const& b) { return std::ranges::is_permutation(a, b); });
});
});
// Set-like = Map-like and Map-like = Set-like
types::for_each(set_like_containers<std::pair<int, int>>{}, []<class To>() {
types::for_each(map_like_containers<int, int>{}, []<class From>() {
test_conversion<From, To>(/* equal = */[](auto const& a, auto const& b) { return std::ranges::is_permutation(a, b); });
test_conversion<To, From>(/* equal = */[](auto const& a, auto const& b) { return std::ranges::is_permutation(a, b); });
});
});
}
```
================
Comment at: libcxx/test/support/almost_satisfies_types.h:61
static_assert(!std::input_iterator<InputIteratorNotIndirectlyReadable>);
-static_assert(!std::ranges::input_range<InputIteratorNotIndirectlyReadable>);
+static_assert(!std::ranges::input_range<InputRangeNotIndirectlyReadable>);
----------------
I think you can make this change and the one below as a NFC before this patch, no review needed.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D142335
Files:
libcxx/docs/FeatureTestMacroTable.rst
libcxx/docs/Status/Cxx2bIssues.csv
libcxx/docs/Status/Cxx2bPapers.csv
libcxx/docs/Status/RangesMajorFeatures.csv
libcxx/include/CMakeLists.txt
libcxx/include/__iterator/ranges_iterator_traits.h
libcxx/include/__ranges/container_compatible_range.h
libcxx/include/__ranges/from_range.h
libcxx/include/__ranges/to.h
libcxx/include/deque
libcxx/include/forward_list
libcxx/include/list
libcxx/include/map
libcxx/include/module.modulemap.in
libcxx/include/queue
libcxx/include/ranges
libcxx/include/set
libcxx/include/stack
libcxx/include/string
libcxx/include/unordered_map
libcxx/include/unordered_set
libcxx/include/vector
libcxx/include/version
libcxx/test/libcxx/private_headers.verify.cpp
libcxx/test/std/containers/sequences/vector/vector.cons/exceptions.pass.cpp
libcxx/test/std/language.support/support.limits/support.limits.general/deque.version.compile.pass.cpp
libcxx/test/std/language.support/support.limits/support.limits.general/list.version.compile.pass.cpp
libcxx/test/std/language.support/support.limits/support.limits.general/stack.version.compile.pass.cpp
libcxx/test/std/language.support/support.limits/support.limits.general/string.version.compile.pass.cpp
libcxx/test/std/language.support/support.limits/support.limits.general/unordered_map.version.compile.pass.cpp
libcxx/test/std/language.support/support.limits/support.limits.general/version.version.compile.pass.cpp
libcxx/test/std/ranges/range.utility/range.utility.conv/to.ctrs.pass.cpp
libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp
libcxx/test/std/ranges/range.utility/range.utility.conv/to_std_containers.pass.cpp
libcxx/utils/generate_feature_test_macro_components.py
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D142335.507651.patch
Type: text/x-patch
Size: 188717 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20230328/cc3fa06f/attachment-0001.bin>
More information about the libcxx-commits
mailing list