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] Fix ranges::to with ADL-only begin/end #119161

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

frederick-vs-ja
Copy link
Contributor

When implementing the resolution of LWG4016, the ranges::for_each call to reduce inclusion dependency. However, the inlining was not always correct, because builtin-in range-for may just stop and fail when a deleted member begin/end function is encountered, while ranges CPOs continue to consider ADL-found begin/end.

As a result, we should still use ranges::begin/ranges::end.

Follows-up #113103. Fixes #119133.

When implementing the resolution of LWG4016, the `ranges::for_each`
call to reduce inclusion dependency. However, the inlining was not
always correct, because builtin-in range-`for` may just stop and fail
when a deleted member `begin`/`end` function is encountered, while
`ranges` CPOs continue to consider ADL-found `begin`/`end`.

As a result, we should still use `ranges::begin`/`ranges::end`.
@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner December 9, 2024 03:34
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Dec 9, 2024
@frederick-vs-ja frederick-vs-ja added the ranges Issues related to `<ranges>` label Dec 9, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 9, 2024

@llvm/pr-subscribers-libcxx

Author: A. Jiang (frederick-vs-ja)

Changes

When implementing the resolution of LWG4016, the ranges::for_each call to reduce inclusion dependency. However, the inlining was not always correct, because builtin-in range-for may just stop and fail when a deleted member begin/end function is encountered, while ranges CPOs continue to consider ADL-found begin/end.

As a result, we should still use ranges::begin/ranges::end.

Follows-up #113103. Fixes #119133.


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

2 Files Affected:

  • (modified) libcxx/include/__ranges/to.h (+5-2)
  • (modified) libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp (+19)
diff --git a/libcxx/include/__ranges/to.h b/libcxx/include/__ranges/to.h
index c937b0656de87d..360c33ee8f2083 100644
--- a/libcxx/include/__ranges/to.h
+++ b/libcxx/include/__ranges/to.h
@@ -109,8 +109,11 @@ template <class _Container, input_range _Range, class... _Args>
         __result.reserve(static_cast<range_size_t<_Container>>(ranges::size(__range)));
       }
 
-      for (auto&& __ref : __range) {
-        using _Ref = decltype(__ref);
+      auto __iter = ranges::begin(__range);
+      auto __sent = ranges::end(__range);
+      for (; __iter != __sent; ++__iter) {
+        auto&& __ref = *__iter;
+        using _Ref   = decltype(__ref);
         if constexpr (requires { __result.emplace_back(std::declval<_Ref>()); }) {
           __result.emplace_back(std::forward<_Ref>(__ref));
         } else if constexpr (requires { __result.push_back(std::declval<_Ref>()); }) {
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 a983745fd636e8..26c7af6d88479b 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
@@ -550,11 +550,30 @@ constexpr void test_recursive() {
   assert(std::ranges::to<C4>(in_owning_view) == result);
 }
 
+struct adl_only_range {
+  static constexpr int numbers[2]{42, 1729};
+
+  void begin() const = delete;
+  void end() const   = delete;
+
+  friend constexpr const int* begin(const adl_only_range&) { return std::ranges::begin(numbers); }
+  friend constexpr const int* end(const adl_only_range&) { return std::ranges::end(numbers); }
+};
+
+constexpr void test_lwg4016_regression() {
+  using Cont = Container<int, CtrChoice::DefaultCtrAndInsert, InserterChoice::PushBack, true>;
+
+  std::ranges::contiguous_range auto r = adl_only_range{};
+  auto v                               = r | std::ranges::to<Cont>();
+  assert(std::ranges::equal(v, adl_only_range::numbers));
+}
+
 constexpr bool test() {
   test_constraints();
   test_ctr_choice_order();
   test_lwg_3785();
   test_recursive();
+  test_lwg4016_regression();
 
   return true;
 }

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. ranges Issues related to `<ranges>`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[libc++] <ranges>: ranges::to use range-based for loops in append branches
2 participants