Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc++][ranges] LWG4016: container-insertable checks do not match what container-inserter does #113103

Merged
merged 4 commits into from
Oct 23, 2024

Conversation

xiaoyang-sde
Copy link
Member

Introduction

This patch implements LWG4016: container-insertable checks do not match what container-inserter does.

Reference

Closes #105322

@xiaoyang-sde xiaoyang-sde requested a review from a team as a code owner October 20, 2024 19:41
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Oct 20, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 20, 2024

@llvm/pr-subscribers-libcxx

Author: Xiaoyang Liu (xiaoyang-sde)

Changes

Introduction

This patch implements LWG4016: container-insertable checks do not match what container-inserter does.

Reference

Closes #105322


Full diff: https://github.com/llvm/llvm-project/pull/113103.diff

4 Files Affected:

  • (modified) libcxx/docs/Status/Cxx2cIssues.csv (+1-1)
  • (modified) libcxx/include/__ranges/to.h (+18-13)
  • (modified) libcxx/test/std/ranges/range.utility/range.utility.conv/container.h (+28-6)
  • (modified) libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp (+44-61)
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>();
   }
 }
 

__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>()); })
Copy link
Member Author

@xiaoyang-sde xiaoyang-sde Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it be better to inline the lambda and the ranges::for_each call by using a plain range-for statement?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ This might indeed be easier to read.

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM as-is, or with @frederick-vs-ja 's comment. I'd leave final approval to @frederick-vs-ja though. Thanks!

__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>()); })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ This might indeed be easier to read.

Copy link
Contributor

@frederick-vs-ja frederick-vs-ja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
(Don't know why CI failed to run, but whatever...)

@ldionne
Copy link
Member

ldionne commented Oct 22, 2024

(Don't know why CI failed to run, but whatever...)

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.

@xiaoyang-sde
Copy link
Member Author

(Don't know why CI failed to run, but whatever...)

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?

@ldionne ldionne merged commit 7c72199 into llvm:main Oct 23, 2024
68 checks passed
@xiaoyang-sde xiaoyang-sde deleted the lwg4016 branch October 23, 2024 17:17
@frobtech frobtech mentioned this pull request Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LWG4016: container-insertable checks do not match what container-inserter does
4 participants