[libc++][ranges] LWG4016: container-insertable checks do not match what container-inserter does #113103
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…at container-inserter does
…at container-inserter does
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Xiaoyang Liu (xiaoyang-sde) ChangesIntroductionThis patch implements LWG4016: container-insertable checks do not match what container-inserter does. ReferenceCloses #105322 Full diff: https://github.com/llvm/llvm-project/pull/113103.diff 4 Files Affected:
diff --git a/libcxx/docs/Status/Cxx2cIssues.csv b/libcxx/docs/Status/Cxx2cIssues.csv
index a62c4992020a0f..6766c69a170a06 100644
--- a/libcxx/docs/Status/Cxx2cIssues.csv
+++ b/libcxx/docs/Status/Cxx2cIssues.csv
@@ -48,7 +48,7 @@
"`LWG4011 <https://wg21.link/LWG4011>`__","``""Effects: Equivalent to return""`` in ``[span.elem]``","2024-03 (Tokyo)","|Nothing To Do|","",""
"`LWG4012 <https://wg21.link/LWG4012>`__","``common_view::begin/end`` are missing the ``simple-view`` check","2024-03 (Tokyo)","","",""
"`LWG4013 <https://wg21.link/LWG4013>`__","``lazy_split_view::outer-iterator::value_type`` should not provide default constructor","2024-03 (Tokyo)","","",""
-"`LWG4016 <https://wg21.link/LWG4016>`__","container-insertable checks do not match what container-inserter does","2024-03 (Tokyo)","","",""
+"`LWG4016 <https://wg21.link/LWG4016>`__","container-insertable checks do not match what container-inserter does","2024-03 (Tokyo)","|Complete|","20.0",""
"`LWG4023 <https://wg21.link/LWG4023>`__","Preconditions of ``std::basic_streambuf::setg/setp``","2024-03 (Tokyo)","|Complete|","19.0",""
"`LWG4025 <https://wg21.link/LWG4025>`__","Move assignment operator of ``std::expected<cv void, E>`` should not be conditionally deleted","2024-03 (Tokyo)","|Complete|","20.0",""
"`LWG4030 <https://wg21.link/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 85fc580b53c9bc..5651db2fc3af4b 100644
--- a/libcxx/include/__ranges/to.h
+++ b/libcxx/include/__ranges/to.h
@@ -10,15 +10,13 @@
#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>
#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>
@@ -54,19 +52,26 @@ constexpr bool __reservable_container =
};
template <class _Container, class _Ref>
-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 <class _Ref, class _Container>
-_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 <class _Container>
+_LIBCPP_HIDE_FROM_ABI constexpr auto __container_append(_Container& __c) {
+ return [&__c]<class _Ref>(_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 +118,13 @@ template <class _Container, input_range _Range, class... _Args>
// 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<range_size_t<_Container>>(ranges::size(__range)));
}
- ranges::copy(__range, ranges::__container_inserter<range_reference_t<_Range>>(__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 ca89e3757affc7..4fd52680add9b3 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 <algorithm>
-#include <concepts>
#include <cstddef>
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 <class T>
+ constexpr void emplace_back(T val)
+ requires(Inserter >= InserterChoice::EmplaceBack)
+ {
+ inserter_choice = InserterChoice::EmplaceBack;
+ __push_back_impl(val);
+ }
+
template <class T>
constexpr void push_back(T val)
requires(Inserter >= InserterChoice::PushBack)
{
inserter_choice = InserterChoice::PushBack;
- buffer_[size_] = val;
+ __push_back_impl(val);
+ }
+
+ template <class T>
+ constexpr void __push_back_impl(T val) {
+ buffer_[size_] = val;
++size_;
}
+ template <class T>
+ constexpr ElementType* emplace(ElementType* where, T val)
+ requires(Inserter >= InserterChoice::Emplace)
+ {
+ inserter_choice = InserterChoice::Emplace;
+ return __insert_impl(where, val);
+ }
+
template <class T>
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 <class T>
+ 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 7f816bb21a1978..a983745fd636e8 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<int, CtrChoice::DefaultCtrAndInsert, InserterChoice::Insert, /*CanReserve=*/false>;
- std::same_as<C> decltype(auto) result = std::ranges::to<C>(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<C>()) == result);
- auto closure = std::ranges::to<C>();
- assert((in | closure) == result);
- }
-
- {
- using C = Container<int, CtrChoice::DefaultCtrAndInsert, InserterChoice::Insert, /*CanReserve=*/true>;
- std::same_as<C> decltype(auto) result = std::ranges::to<C>(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<C>()) == result);
- auto closure = std::ranges::to<C>();
- assert((in | closure) == result);
- }
-
- {
- using C = Container<int, CtrChoice::DefaultCtrAndInsert, InserterChoice::PushBack, /*CanReserve=*/false>;
- std::same_as<C> decltype(auto) result = std::ranges::to<C>(in);
+ auto case_4 = [in, arg1, arg2]<auto InserterChoice, bool CanReserve>() {
+ using C = Container<int, CtrChoice::DefaultCtrAndInsert, InserterChoice, CanReserve>;
+ {
+ [[maybe_unused]] std::same_as<C> decltype(auto) result = std::ranges::to<C>(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<C>()) == result);
- auto closure = std::ranges::to<C>();
- assert((in | closure) == result);
- }
+ assert((in | std::ranges::to<C>()) == result);
+ [[maybe_unused]] auto closure = std::ranges::to<C>();
+ assert((in | closure) == result);
+ }
- {
- using C = Container<int, CtrChoice::DefaultCtrAndInsert, InserterChoice::PushBack, /*CanReserve=*/true>;
- std::same_as<C> decltype(auto) result = std::ranges::to<C>(in);
+ { // Extra arguments
+ [[maybe_unused]] std::same_as<C> decltype(auto) result = std::ranges::to<C>(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<C>()) == result);
- auto closure = std::ranges::to<C>();
- 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<int, CtrChoice::DefaultCtrAndInsert, InserterChoice::Insert, /*CanReserve=*/false>;
- std::same_as<C> decltype(auto) result = std::ranges::to<C>(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<C>(arg1, arg2)) == result);
- auto closure = std::ranges::to<C>(arg1, arg2);
- assert((in | closure) == result);
- }
+ assert((in | std::ranges::to<C>(arg1, arg2)) == result);
+ [[maybe_unused]] auto closure = std::ranges::to<C>(arg1, arg2);
+ assert((in | closure) == result);
+ }
+ };
+
+ case_4.operator()<InserterChoice::Insert, false>();
+ case_4.operator()<InserterChoice::Insert, true>();
+ case_4.operator()<InserterChoice::Emplace, false>();
+ case_4.operator()<InserterChoice::Emplace, true>();
+ case_4.operator()<InserterChoice::PushBack, false>();
+ case_4.operator()<InserterChoice::PushBack, true>();
+ case_4.operator()<InserterChoice::EmplaceBack, false>();
+ case_4.operator()<InserterChoice::EmplaceBack, true>();
}
}
|
xiaoyang-sde
commented
Oct 21, 2024
libcxx/include/__ranges/to.h
Outdated
| __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>()); }) |
There was a problem hiding this comment.
A full if constexpr condition is required instead of just using else, as Clang has not yet implemented P0588R1. Without this proposal, when the function template __container_append is instantiated, Clang partially instantiates the body of the returned generic lambda. (I believe it needs to perform instantiation to determine the set of captures.) Using else in this scenario causes Clang to attempt evaluating __c.insert(__c.end(), std::forward<_Ref>(__ref)); even though the type of _Ref remains unknown, which can result in an error if __c lacks a valid insert method.
However, with P0588R1, Clang can defer this instantiation until the body of the generic lambda is actually invoked. At that moment, the type of _Ref is known, so the statement __c.insert(__c.end(), std::forward<_Ref>(__ref)); can be discarded.
[Note 5: The instantiation of a generic lambda does not require instantiation of substatements of a constexpr if statement within its compound-statement unless the call operator template is instantiated. — end note]
There was a problem hiding this comment.
Will it be better to inline the lambda and the ranges::for_each call by using a plain range-for statement?
There was a problem hiding this comment.
^ This might indeed be easier to read.
ldionne
approved these changes
Oct 21, 2024
There was a problem hiding this comment.
This LGTM as-is, or with @frederick-vs-ja 's comment. I'd leave final approval to @frederick-vs-ja though. Thanks!
libcxx/include/__ranges/to.h
Outdated
| __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>()); }) |
There was a problem hiding this comment.
^ This might indeed be easier to read.
…at container-inserter does
…at container-inserter does
frederick-vs-ja
approved these changes
Oct 22, 2024
Unless we can confirm that it's a fluke or an unrelated failure, we shouldn't merge until the CI is green. Our CI is usually pretty stable and a CI failure generally means a true failure. However, when the CI jobs start, they fail due to "interrupted by user" often and they get automatically restarted. When you see one of these failures, it basically means that the CI is not done running yet and that job needs to be restarted (which will happen automatically). What's going on under the hood is that our CI machines are containers that can be preempted so the jobs sometimes get killed out of nowhere. |
Thanks! Could you please merge this patch? |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduction
This patch implements LWG4016: container-insertable checks do not match what container-inserter does.
Reference
Closes #105322