From 97d7316b5118c4e703ba3c43a35676b325df961a Mon Sep 17 00:00:00 2001 From: Xiaoyang Liu Date: Sun, 20 Oct 2024 15:36:47 -0400 Subject: [PATCH 1/4] [libc++][ranges] LWG4016: container-insertable checks do not match what container-inserter does --- libcxx/docs/Status/Cxx2cIssues.csv | 2 +- libcxx/include/__ranges/to.h | 29 +++-- .../range.utility.conv/container.h | 34 +++++- .../range.utility.conv/to.pass.cpp | 105 ++++++++---------- 4 files changed, 91 insertions(+), 79 deletions(-) diff --git a/libcxx/docs/Status/Cxx2cIssues.csv b/libcxx/docs/Status/Cxx2cIssues.csv index a62c4992020a0..6766c69a170a0 100644 --- a/libcxx/docs/Status/Cxx2cIssues.csv +++ b/libcxx/docs/Status/Cxx2cIssues.csv @@ -48,7 +48,7 @@ "`LWG4011 `__","``""Effects: Equivalent to return""`` in ``[span.elem]``","2024-03 (Tokyo)","|Nothing To Do|","","" "`LWG4012 `__","``common_view::begin/end`` are missing the ``simple-view`` check","2024-03 (Tokyo)","","","" "`LWG4013 `__","``lazy_split_view::outer-iterator::value_type`` should not provide default constructor","2024-03 (Tokyo)","","","" -"`LWG4016 `__","container-insertable checks do not match what container-inserter does","2024-03 (Tokyo)","","","" +"`LWG4016 `__","container-insertable checks do not match what container-inserter does","2024-03 (Tokyo)","|Complete|","20.0","" "`LWG4023 `__","Preconditions of ``std::basic_streambuf::setg/setp``","2024-03 (Tokyo)","|Complete|","19.0","" "`LWG4025 `__","Move assignment operator of ``std::expected`` should not be conditionally deleted","2024-03 (Tokyo)","|Complete|","20.0","" "`LWG4030 `__","Clarify whether arithmetic expressions in ``[numeric.sat.func]`` are mathematical or C++","2024-03 (Tokyo)","|Nothing To Do|","","" diff --git a/libcxx/include/__ranges/to.h b/libcxx/include/__ranges/to.h index 85fc580b53c9b..0bb18cc330cd6 100644 --- a/libcxx/include/__ranges/to.h +++ b/libcxx/include/__ranges/to.h @@ -10,7 +10,7 @@ #ifndef _LIBCPP___RANGES_TO_H #define _LIBCPP___RANGES_TO_H -#include <__algorithm/ranges_copy.h> +#include <__algorithm/ranges_for_each.h> #include <__concepts/constructible.h> #include <__concepts/convertible_to.h> #include <__concepts/derived_from.h> @@ -54,19 +54,26 @@ constexpr bool __reservable_container = }; template -constexpr bool __container_insertable = requires(_Container& __c, _Ref&& __ref) { +constexpr bool __container_appendable = requires(_Container& __c, _Ref&& __ref) { requires( + requires { __c.emplace_back(std::forward<_Ref>(__ref)); } || requires { __c.push_back(std::forward<_Ref>(__ref)); } || + requires { __c.emplace(__c.end(), std::forward<_Ref>(__ref)); } || requires { __c.insert(__c.end(), std::forward<_Ref>(__ref)); }); }; -template -_LIBCPP_HIDE_FROM_ABI constexpr auto __container_inserter(_Container& __c) { - if constexpr (requires { __c.push_back(std::declval<_Ref>()); }) { - return std::back_inserter(__c); - } else { - return std::inserter(__c, __c.end()); - } +template +_LIBCPP_HIDE_FROM_ABI constexpr auto __container_append(_Container& __c) { + return [&__c](_Ref&& __ref) { + if constexpr (requires { __c.emplace_back(declval<_Ref>()); }) + __c.emplace_back(std::forward<_Ref>(__ref)); + else if constexpr (requires { __c.push_back(declval<_Ref>()); }) + __c.push_back(std::forward<_Ref>(__ref)); + else if constexpr (requires { __c.emplace(__c.end(), declval<_Ref>()); }) + __c.emplace(__c.end(), std::forward<_Ref>(__ref)); + else if constexpr (requires { __c.insert(__c.end(), declval<_Ref>()); }) + __c.insert(__c.end(), std::forward<_Ref>(__ref)); + }; } // Note: making this a concept allows short-circuiting the second condition. @@ -113,13 +120,13 @@ template // Case 4 -- default-construct (or construct from the extra arguments) and insert, reserving the size if possible. else if constexpr (constructible_from<_Container, _Args...> && - __container_insertable<_Container, range_reference_t<_Range>>) { + __container_appendable<_Container, range_reference_t<_Range>>) { _Container __result(std::forward<_Args>(__args)...); if constexpr (sized_range<_Range> && __reservable_container<_Container>) { __result.reserve(static_cast>(ranges::size(__range))); } - ranges::copy(__range, ranges::__container_inserter>(__result)); + ranges::for_each(__range, ranges::__container_append(__result)); return __result; diff --git a/libcxx/test/std/ranges/range.utility/range.utility.conv/container.h b/libcxx/test/std/ranges/range.utility/range.utility.conv/container.h index ca89e3757affc..4fd52680add9b 100644 --- a/libcxx/test/std/ranges/range.utility/range.utility.conv/container.h +++ b/libcxx/test/std/ranges/range.utility/range.utility.conv/container.h @@ -10,12 +10,11 @@ #define RANGES_RANGE_UTILITY_RANGE_UTILITY_CONV_CONTAINER_H #include -#include #include enum class CtrChoice { Invalid, DefaultCtrAndInsert, BeginEndPair, FromRangeT, DirectCtr }; -enum class InserterChoice { Invalid, Insert, PushBack }; +enum class InserterChoice { Invalid, Insert, Emplace, PushBack, EmplaceBack }; // Allows checking that `ranges::to` correctly follows the order of priority of different constructors -- e.g., if // 3 constructors are available, the `from_range_t` constructor is chosen in favor of the constructor taking two @@ -96,27 +95,50 @@ struct Container { constexpr ElementType* end() { return buffer_ + size_; } constexpr std::size_t size() const { return size_; } + template + constexpr void emplace_back(T val) + requires(Inserter >= InserterChoice::EmplaceBack) + { + inserter_choice = InserterChoice::EmplaceBack; + __push_back_impl(val); + } + template constexpr void push_back(T val) requires(Inserter >= InserterChoice::PushBack) { inserter_choice = InserterChoice::PushBack; - buffer_[size_] = val; + __push_back_impl(val); + } + + template + constexpr void __push_back_impl(T val) { + buffer_[size_] = val; ++size_; } + template + constexpr ElementType* emplace(ElementType* where, T val) + requires(Inserter >= InserterChoice::Emplace) + { + inserter_choice = InserterChoice::Emplace; + return __insert_impl(where, val); + } + template constexpr ElementType* insert(ElementType* where, T val) requires(Inserter >= InserterChoice::Insert) { - assert(size() + 1 <= Capacity); - inserter_choice = InserterChoice::Insert; + return __insert_impl(where, val); + } + template + constexpr ElementType* __insert_impl(ElementType* where, T val) { + assert(size() + 1 <= Capacity); std::shift_right(where, end(), 1); *where = val; ++size_; - return where; } diff --git a/libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp b/libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp index 7f816bb21a197..a983745fd636e 100644 --- a/libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp +++ b/libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp @@ -392,72 +392,55 @@ constexpr void test_ctr_choice_order() { } { // Case 4 -- default-construct then insert elements. - { - using C = Container; - std::same_as decltype(auto) result = std::ranges::to(in); - - assert(result.ctr_choice == CtrChoice::DefaultCtrAndInsert); - assert(result.inserter_choice == InserterChoice::Insert); - assert(std::ranges::equal(result, in)); - assert(!result.called_reserve); - assert((in | std::ranges::to()) == result); - auto closure = std::ranges::to(); - assert((in | closure) == result); - } - - { - using C = Container; - std::same_as decltype(auto) result = std::ranges::to(in); - - assert(result.ctr_choice == CtrChoice::DefaultCtrAndInsert); - assert(result.inserter_choice == InserterChoice::Insert); - assert(std::ranges::equal(result, in)); - assert(result.called_reserve); - assert((in | std::ranges::to()) == result); - auto closure = std::ranges::to(); - assert((in | closure) == result); - } - - { - using C = Container; - std::same_as decltype(auto) result = std::ranges::to(in); + auto case_4 = [in, arg1, arg2]() { + using C = Container; + { + [[maybe_unused]] std::same_as decltype(auto) result = std::ranges::to(in); + + assert(result.ctr_choice == CtrChoice::DefaultCtrAndInsert); + assert(result.inserter_choice == InserterChoice); + assert(std::ranges::equal(result, in)); + + if constexpr (CanReserve) { + assert(result.called_reserve); + } else { + assert(!result.called_reserve); + } - assert(result.ctr_choice == CtrChoice::DefaultCtrAndInsert); - assert(result.inserter_choice == InserterChoice::PushBack); - assert(std::ranges::equal(result, in)); - assert(!result.called_reserve); - assert((in | std::ranges::to()) == result); - auto closure = std::ranges::to(); - assert((in | closure) == result); - } + assert((in | std::ranges::to()) == result); + [[maybe_unused]] auto closure = std::ranges::to(); + assert((in | closure) == result); + } - { - using C = Container; - std::same_as decltype(auto) result = std::ranges::to(in); + { // Extra arguments + [[maybe_unused]] std::same_as decltype(auto) result = std::ranges::to(in, arg1, arg2); - assert(result.ctr_choice == CtrChoice::DefaultCtrAndInsert); - assert(result.inserter_choice == InserterChoice::PushBack); - assert(std::ranges::equal(result, in)); - assert(result.called_reserve); - assert((in | std::ranges::to()) == result); - auto closure = std::ranges::to(); - assert((in | closure) == result); - } + assert(result.ctr_choice == CtrChoice::DefaultCtrAndInsert); + assert(result.inserter_choice == InserterChoice); + assert(std::ranges::equal(result, in)); + assert(result.extra_arg1 == arg1); + assert(result.extra_arg2 == arg2); - { // Extra arguments. - using C = Container; - std::same_as decltype(auto) result = std::ranges::to(in, arg1, arg2); + if constexpr (CanReserve) { + assert(result.called_reserve); + } else { + assert(!result.called_reserve); + } - assert(result.ctr_choice == CtrChoice::DefaultCtrAndInsert); - assert(result.inserter_choice == InserterChoice::Insert); - assert(std::ranges::equal(result, in)); - assert(!result.called_reserve); - assert(result.extra_arg1 == arg1); - assert(result.extra_arg2 == arg2); - assert((in | std::ranges::to(arg1, arg2)) == result); - auto closure = std::ranges::to(arg1, arg2); - assert((in | closure) == result); - } + assert((in | std::ranges::to(arg1, arg2)) == result); + [[maybe_unused]] auto closure = std::ranges::to(arg1, arg2); + assert((in | closure) == result); + } + }; + + case_4.operator()(); + case_4.operator()(); + case_4.operator()(); + case_4.operator()(); + case_4.operator()(); + case_4.operator()(); + case_4.operator()(); + case_4.operator()(); } } From 482b830a99bee541213728dd2f03c6e95c16200d Mon Sep 17 00:00:00 2001 From: Xiaoyang Liu Date: Sun, 20 Oct 2024 15:40:31 -0400 Subject: [PATCH 2/4] [libc++][ranges] LWG4016: container-insertable checks do not match what container-inserter does --- libcxx/include/__ranges/to.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/libcxx/include/__ranges/to.h b/libcxx/include/__ranges/to.h index 0bb18cc330cd6..5651db2fc3af4 100644 --- a/libcxx/include/__ranges/to.h +++ b/libcxx/include/__ranges/to.h @@ -17,8 +17,6 @@ #include <__concepts/same_as.h> #include <__config> #include <__functional/bind_back.h> -#include <__iterator/back_insert_iterator.h> -#include <__iterator/insert_iterator.h> #include <__iterator/iterator_traits.h> #include <__ranges/access.h> #include <__ranges/concepts.h> From a80c61cd02a714314026d3c17ff6fe46394a4d8e Mon Sep 17 00:00:00 2001 From: Xiaoyang Liu Date: Mon, 21 Oct 2024 21:26:17 -0400 Subject: [PATCH 3/4] [libc++][ranges] LWG4016: container-insertable checks do not match what container-inserter does --- libcxx/include/__ranges/to.h | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/libcxx/include/__ranges/to.h b/libcxx/include/__ranges/to.h index 5651db2fc3af4..510d499f7f00a 100644 --- a/libcxx/include/__ranges/to.h +++ b/libcxx/include/__ranges/to.h @@ -60,20 +60,6 @@ constexpr bool __container_appendable = requires(_Container& __c, _Ref&& __ref) requires { __c.insert(__c.end(), std::forward<_Ref>(__ref)); }); }; -template -_LIBCPP_HIDE_FROM_ABI constexpr auto __container_append(_Container& __c) { - return [&__c](_Ref&& __ref) { - if constexpr (requires { __c.emplace_back(declval<_Ref>()); }) - __c.emplace_back(std::forward<_Ref>(__ref)); - else if constexpr (requires { __c.push_back(declval<_Ref>()); }) - __c.push_back(std::forward<_Ref>(__ref)); - else if constexpr (requires { __c.emplace(__c.end(), declval<_Ref>()); }) - __c.emplace(__c.end(), std::forward<_Ref>(__ref)); - else if constexpr (requires { __c.insert(__c.end(), declval<_Ref>()); }) - __c.insert(__c.end(), std::forward<_Ref>(__ref)); - }; -} - // Note: making this a concept allows short-circuiting the second condition. template concept __try_non_recursive_conversion = @@ -124,8 +110,19 @@ template __result.reserve(static_cast>(ranges::size(__range))); } - ranges::for_each(__range, ranges::__container_append(__result)); - + for (auto&& __ref : __range) { + using _Ref = decltype(__ref); + if constexpr (requires { __result.emplace_back(declval<_Ref>()); }) { + __result.emplace_back(std::forward<_Ref>(__ref)); + } else if constexpr (requires { __result.push_back(declval<_Ref>()); }) { + __result.push_back(std::forward<_Ref>(__ref)); + } else if constexpr (requires { __result.emplace(__result.end(), declval<_Ref>()); }) { + __result.emplace(__result.end(), std::forward<_Ref>(__ref)); + } else { + static_assert(requires { __result.insert(__result.end(), declval<_Ref>()); }); + __result.insert(__result.end(), std::forward<_Ref>(__ref)); + } + } return __result; } else { From fd03741315a17ccef0af54224264454157b9f0db Mon Sep 17 00:00:00 2001 From: Xiaoyang Liu Date: Mon, 21 Oct 2024 22:33:39 -0400 Subject: [PATCH 4/4] [libc++][ranges] LWG4016: container-insertable checks do not match what container-inserter does --- libcxx/include/__ranges/to.h | 1 - 1 file changed, 1 deletion(-) diff --git a/libcxx/include/__ranges/to.h b/libcxx/include/__ranges/to.h index 510d499f7f00a..52666075da3e2 100644 --- a/libcxx/include/__ranges/to.h +++ b/libcxx/include/__ranges/to.h @@ -10,7 +10,6 @@ #ifndef _LIBCPP___RANGES_TO_H #define _LIBCPP___RANGES_TO_H -#include <__algorithm/ranges_for_each.h> #include <__concepts/constructible.h> #include <__concepts/convertible_to.h> #include <__concepts/derived_from.h>