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] LWG3984: ranges::to's recursion branch may be ill-formed #87964

Merged
merged 3 commits into from
Apr 26, 2024

Conversation

xiaoyang-sde
Copy link
Member

@xiaoyang-sde xiaoyang-sde commented Apr 8, 2024

Abstract

This pull request implements LWG3984: ranges::to's recursion branch may be ill-formed.

In the current implementation, ranges::to's recursion branch pipes the range into a views::transform(/* lambda */), which is a __range_adaptor_closure object. In libc++, the pipe operator (operator|) of __range_adaptor_closure requires a viewable_range, so the following code won't compile, as the type of lvalue r doesn't model viewable_range:

#include <ranges>
#include <vector>
#include <list>

int main() {
  std::vector<std::vector<int>> v;
  auto r = std::views::all(std::move(v));
  auto l = std::ranges::to<std::list<std::list<int>>>(r);
}

Reference

@xiaoyang-sde xiaoyang-sde requested a review from a team as a code owner April 8, 2024 07:28
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Apr 8, 2024
@llvmbot
Copy link
Member

llvmbot commented Apr 8, 2024

@llvm/pr-subscribers-libcxx

Author: Xiaoyang Liu (xiaoyang-sde)

Changes

Abstract

This pull request implements LWG3984: ranges::to's recursion branch may be ill-formed.

In the current implementation, ranges::to's recursion branch pipes the range into a views::transform(/* lambda */), which is a __range_adaptor_closure object. In libc++, the pipe operator (operator|) of __range_adaptor_closure requires a viewable_range, so the following code won't compile, as the type of lvalue r doesn't model viewable_range:

#include &lt;ranges&gt;
#include &lt;vector&gt;
#include &lt;list&gt;

int main() {
  std::vector&lt;std::vector&lt;int&gt;&gt; v;
  auto r = std::views::all(std::move(v));
  auto l = std::ranges::to&lt;std::list&lt;std::list&lt;int&gt;&gt;&gt;(r);
}

Reference


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

3 Files Affected:

  • (modified) libcxx/docs/Status/Cxx2cIssues.csv (+1-1)
  • (modified) libcxx/include/__ranges/to.h (+2-1)
  • (modified) libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp (+5)
diff --git a/libcxx/docs/Status/Cxx2cIssues.csv b/libcxx/docs/Status/Cxx2cIssues.csv
index 8a4bf2ef62162a..d91fff7c8687e0 100644
--- a/libcxx/docs/Status/Cxx2cIssues.csv
+++ b/libcxx/docs/Status/Cxx2cIssues.csv
@@ -44,7 +44,7 @@
 "`3919 <https://wg21.link/LWG3919>`__","``enumerate_view`` may invoke UB for sized common non-forward underlying ranges","Tokyo March 2024","","","|ranges|"
 "`3950 <https://wg21.link/LWG3950>`__","``std::basic_string_view`` comparison operators are overspecified","Tokyo March 2024","|Complete|","18.0",""
 "`3975 <https://wg21.link/LWG3975>`__","Specializations of ``basic_format_context`` should not be permitted","Tokyo March 2024","|Nothing To Do|","","|format|"
-"`3984 <https://wg21.link/LWG3984>`__","``ranges::to``'s recursion branch may be ill-formed","Tokyo March 2024","","","|ranges|"
+"`3984 <https://wg21.link/LWG3984>`__","``ranges::to``'s recursion branch may be ill-formed","Tokyo March 2024","|Complete|","19.0","|ranges|"
 "`4011 <https://wg21.link/LWG4011>`__","``""Effects: Equivalent to return""`` in ``[span.elem]``","Tokyo March 2024","|Nothing To Do|","",""
 "`4012 <https://wg21.link/LWG4012>`__","``common_view::begin/end`` are missing the ``simple-view`` check","Tokyo March 2024","","","|ranges|"
 "`4013 <https://wg21.link/LWG4013>`__","``lazy_split_view::outer-iterator::value_type`` should not provide default constructor","Tokyo March 2024","","","|ranges|"
diff --git a/libcxx/include/__ranges/to.h b/libcxx/include/__ranges/to.h
index 67818c521b1500..7d042b196f6a52 100644
--- a/libcxx/include/__ranges/to.h
+++ b/libcxx/include/__ranges/to.h
@@ -24,6 +24,7 @@
 #include <__ranges/concepts.h>
 #include <__ranges/from_range.h>
 #include <__ranges/range_adaptor.h>
+#include <__ranges/ref_view.h>
 #include <__ranges/size.h>
 #include <__ranges/transform_view.h>
 #include <__type_traits/add_pointer.h>
@@ -129,7 +130,7 @@ _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI constexpr _Container to(_Range&& __r
     // Try the recursive case.
   } else if constexpr (input_range<range_reference_t<_Range>>) {
     return ranges::to<_Container>(
-        __range | views::transform([](auto&& __elem) {
+        ranges::ref_view(__range) | views::transform([](auto&& __elem) {
           return ranges::to<range_value_t<_Container>>(std::forward<decltype(__elem)>(__elem));
         }),
         std::forward<_Args>(__args)...);
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 3df88d6a2dcc38..7f816bb21a1978 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
@@ -560,6 +560,11 @@ constexpr void test_recursive() {
   }
 
   assert((in | std::ranges::to<C4>()) == result);
+
+  // LWG3984: ranges::to's recursion branch may be ill-formed
+  auto in_owning_view = std::views::all(std::move(in));
+  static_assert(!std::ranges::viewable_range<decltype((in_owning_view))>);
+  assert(std::ranges::to<C4>(in_owning_view) == result);
 }
 
 constexpr bool test() {

…formed

Co-authored-by: A. Jiang <de34@live.cn>
@ldionne ldionne added the ranges Issues related to `<ranges>` label Apr 26, 2024
@ldionne ldionne assigned var-const and ldionne and unassigned var-const Apr 26, 2024
@ldionne
Copy link
Member

ldionne commented Apr 26, 2024

^ Actually this is small and I already reviewed it, I'll take this one :)

@ldionne ldionne merged commit e74be35 into llvm:main Apr 26, 2024
54 checks passed
@xiaoyang-sde xiaoyang-sde deleted the ranges_to branch April 26, 2024 15:07
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.

5 participants