[libc++] `views::split` and `views::lazy_split` shouldn't be range adaptor closures by StephanTLavavej · Pull Request #75266 · llvm/llvm-project

StephanTLavavej · 2023-12-13T01:01:48Z

Fixes #75002. Found while running libc++'s tests with MSVC's STL.

This is a superset of #74961 that also attempts to fix the product code and add a regression test. Thanks again, @cpplearner!

Click to expand Standardese citations:

N4964 [range.split.overview]/2:

The name views::split denotes a range adaptor object ([range.adaptor.object]). Given subexpressions E and F, the expression views::split(E, F) is expression-equivalent to split_view(E, F).
[range.lazy.split.overview]/2:

The name views::lazy_split denotes a range adaptor object ([range.adaptor.object]). Given subexpressions E and F, the expression views::lazy_split(E, F) is expression-equivalent to lazy_split_view(E, F).
[range.adaptor.object]/1:

A range adaptor closure object is a unary function object that accepts a range argument. For a range adaptor
closure object C and an expression R such that decltype((R)) models range, the following expressions are
equivalent:
C(R)
R | C
[range.adaptor.object]/6-8:

A range adaptor object is a customization point object ([customization.point.object]) that accepts a viewable_range as its first argument and returns a view.
If a range adaptor object accepts only one argument, then it is a range adaptor closure object.
If a range adaptor object adaptor accepts more than one argument, then let range be an expression such that decltype((range)) models viewable_range, let args... be arguments such that adaptor(range, args...) is a well-formed expression as specified in the rest of subclause [range.adaptors], and let BoundArgs be a pack that denotes decay_t<decltype((args))>.... The expression adaptor(args...) produces a range adaptor closure object f that is a perfect forwarding call wrapper ([func.require]) with the following properties: [...]

To summarize: views::split and views::lazy_split aren't unary, aren't range adaptor closure objects, and can't be piped. However, [range.adaptor.object]/8 says that views::split(pattern) and views::lazy_split(pattern) produce unary, pipeable, range adaptor closure objects.

This PR adjusts the test coverage accordingly, allowing it to portably pass for libc++ and MSVC's STL.

Click to expand the Clang compiler error that discovered this:

D:\GitHub\STL\llvm-project\libcxx\test\std\ranges\range.adaptors\range.split\adaptor.pass.cpp(42,1): error: static assertion failed
static_assert( CanBePiped<SomeView&,    decltype(std::views::split)>);
^              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
D:\GitHub\STL\llvm-project\libcxx\test\std\ranges\range.adaptors\range.split\adaptor.pass.cpp(42,16): note: because 'CanBePiped<SomeView &, decltype(std::views::split)>' evaluated to false
static_assert( CanBePiped<SomeView&,    decltype(std::views::split)>);
                ^
D:\GitHub\STL\llvm-project\libcxx\test\std\ranges\range.adaptors\range.split\adaptor.pass.cpp(25,30): note: because 'std::forward<View>(view) | std::forward<T>(t)' would be invalid: invalid operands to binary expression ('SomeView' and 'const std::ranges::views::_Split_fn')
  { std::forward<View>(view) | std::forward<T>(t) };
                              ^
D:\GitHub\STL\llvm-project\libcxx\test\std\ranges\range.adaptors\range.split\adaptor.pass.cpp(43,1): error: static assertion failed
static_assert( CanBePiped<char(&)[10],  decltype(std::views::split)>);
^              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
D:\GitHub\STL\llvm-project\libcxx\test\std\ranges\range.adaptors\range.split\adaptor.pass.cpp(43,16): note: because 'CanBePiped<char (&)[10], decltype(std::views::split)>' evaluated to false
static_assert( CanBePiped<char(&)[10],  decltype(std::views::split)>);
                ^
D:\GitHub\STL\llvm-project\libcxx\test\std\ranges\range.adaptors\range.split\adaptor.pass.cpp(25,30): note: because 'std::forward<View>(view) | std::forward<T>(t)' would be invalid: invalid operands to binary expression ('char[10]' and 'const std::ranges::views::_Split_fn')
  { std::forward<View>(view) | std::forward<T>(t) };
                              ^

This was failing with: D:\GitHub\STL\llvm-project\libcxx\test\std\ranges\range.adaptors\range.split\adaptor.pass.cpp(42,1): error: static assertion failed static_assert( CanBePiped<SomeView&, decltype(std::views::split)>); ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ D:\GitHub\STL\llvm-project\libcxx\test\std\ranges\range.adaptors\range.split\adaptor.pass.cpp(42,16): note: because 'CanBePiped<SomeView &, decltype(std::views::split)>' evaluated to false static_assert( CanBePiped<SomeView&, decltype(std::views::split)>); ^ D:\GitHub\STL\llvm-project\libcxx\test\std\ranges\range.adaptors\range.split\adaptor.pass.cpp(25,30): note: because 'std::forward<View>(view) | std::forward<T>(t)' would be invalid: invalid operands to binary expression ('SomeView' and 'const std::ranges::views::_Split_fn') { std::forward<View>(view) | std::forward<T>(t) }; ^ D:\GitHub\STL\llvm-project\libcxx\test\std\ranges\range.adaptors\range.split\adaptor.pass.cpp(43,1): error: static assertion failed static_assert( CanBePiped<char(&)[10], decltype(std::views::split)>); ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ D:\GitHub\STL\llvm-project\libcxx\test\std\ranges\range.adaptors\range.split\adaptor.pass.cpp(43,16): note: because 'CanBePiped<char (&)[10], decltype(std::views::split)>' evaluated to false static_assert( CanBePiped<char(&)[10], decltype(std::views::split)>); ^ D:\GitHub\STL\llvm-project\libcxx\test\std\ranges\range.adaptors\range.split\adaptor.pass.cpp(25,30): note: because 'std::forward<View>(view) | std::forward<T>(t)' would be invalid: invalid operands to binary expression ('char[10]' and 'const std::ranges::views::_Split_fn') { std::forward<View>(view) | std::forward<T>(t) }; ^ Similarly for range.lazy.split/adaptor.pass.cpp.

llvmbot · 2023-12-13T01:02:14Z

@llvm/pr-subscribers-libcxx

Author: Stephan T. Lavavej (StephanTLavavej)

Changes

Fixes #75002. Found while running libc++'s tests with MSVC's STL.

This is a superset of #74961 that also attempts to fix the product code and add a regression test. Thanks again, @cpplearner!

<details><summary>Click to expand Standardese citations:</summary>

N4964 [range.split.overview]/2:
> The name views::split denotes a range adaptor object ([range.adaptor.object]). Given subexpressions E and F, the expression views::split(E, F) is expression-equivalent to split_view(E, F).
[range.lazy.split.overview]/2:
> The name views::lazy_split denotes a range adaptor object ([range.adaptor.object]). Given subexpressions E and F, the expression views::lazy_split(E, F) is expression-equivalent to lazy_split_view(E, F).
[range.adaptor.object]/1:
> A range adaptor closure object is a unary function object that accepts a range argument. For a range adaptor
closure object C and an expression R such that decltype((R)) models range, the following expressions are
equivalent:
> C(R)
> R | C
[range.adaptor.object]/6-8:
> A range adaptor object is a customization point object ([customization.point.object]) that accepts a viewable_range as its first argument and returns a view.
> If a range adaptor object accepts only one argument, then it is a range adaptor closure object.
> If a range adaptor object adaptor accepts more than one argument, then let range be an expression such that decltype((range)) models viewable_range, let args... be arguments such that adaptor(range, args...) is a well-formed expression as specified in the rest of subclause [range.adaptors], and let BoundArgs be a pack that denotes decay_t<decltype((args))>.... The expression adaptor(args...) produces a range adaptor closure object f that is a perfect forwarding call wrapper ([func.require]) with the following properties: [...]
</details>

To summarize: views::split and views::lazy_split aren't unary, aren't range adaptor closure objects, and can't be piped. However, [range.adaptor.object]/8 says that views::split(pattern) and views::lazy_split(pattern) produce unary, pipeable, range adaptor closure objects.

This PR adjusts the test coverage accordingly, allowing it to portably pass for libc++ and MSVC's STL.

<details><summary>Click to expand the Clang compiler error that discovered this:</summary>

D:\GitHub\STL\llvm-project\libcxx\test\std\ranges\range.adaptors\range.split\adaptor.pass.cpp(42,1): error: static assertion failed
static_assert( CanBePiped&lt;SomeView&amp;,    decltype(std::views::split)&gt;);
^              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
D:\GitHub\STL\llvm-project\libcxx\test\std\ranges\range.adaptors\range.split\adaptor.pass.cpp(42,16): note: because 'CanBePiped&lt;SomeView &amp;, decltype(std::views::split)&gt;' evaluated to false
static_assert( CanBePiped&lt;SomeView&amp;,    decltype(std::views::split)&gt;);
                ^
D:\GitHub\STL\llvm-project\libcxx\test\std\ranges\range.adaptors\range.split\adaptor.pass.cpp(25,30): note: because 'std::forward&lt;View&gt;(view) | std::forward&lt;T&gt;(t)' would be invalid: invalid operands to binary expression ('SomeView' and 'const std::ranges::views::_Split_fn')
  { std::forward&lt;View&gt;(view) | std::forward&lt;T&gt;(t) };
                              ^
D:\GitHub\STL\llvm-project\libcxx\test\std\ranges\range.adaptors\range.split\adaptor.pass.cpp(43,1): error: static assertion failed
static_assert( CanBePiped&lt;char(&amp;)[10],  decltype(std::views::split)&gt;);
^              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
D:\GitHub\STL\llvm-project\libcxx\test\std\ranges\range.adaptors\range.split\adaptor.pass.cpp(43,16): note: because 'CanBePiped&lt;char (&amp;)[10], decltype(std::views::split)&gt;' evaluated to false
static_assert( CanBePiped&lt;char(&amp;)[10],  decltype(std::views::split)&gt;);
                ^
D:\GitHub\STL\llvm-project\libcxx\test\std\ranges\range.adaptors\range.split\adaptor.pass.cpp(25,30): note: because 'std::forward&lt;View&gt;(view) | std::forward&lt;T&gt;(t)' would be invalid: invalid operands to binary expression ('char[10]' and 'const std::ranges::views::_Split_fn')
  { std::forward&lt;View&gt;(view) | std::forward&lt;T&gt;(t) };
                              ^

</details>

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

4 Files Affected:

(modified) libcxx/include/__ranges/lazy_split_view.h (+1-1)
(modified) libcxx/include/__ranges/split_view.h (+1-1)
(modified) libcxx/test/std/ranges/range.adaptors/range.lazy.split/adaptor.pass.cpp (+10-4)
(modified) libcxx/test/std/ranges/range.adaptors/range.split/adaptor.pass.cpp (+10-4)

diff --git a/libcxx/include/__ranges/lazy_split_view.h b/libcxx/include/__ranges/lazy_split_view.h
index 2c654bfd325a63..8ed4bcfdeb56d4 100644
--- a/libcxx/include/__ranges/lazy_split_view.h
+++ b/libcxx/include/__ranges/lazy_split_view.h
@@ -437,7 +437,7 @@ lazy_split_view(_Range&&, range_value_t<_Range>)
 
 namespace views {
 namespace __lazy_split_view {
-struct __fn : __range_adaptor_closure<__fn> {
+struct __fn {
   template <class _Range, class _Pattern>
   [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
   constexpr auto operator()(_Range&& __range, _Pattern&& __pattern) const
diff --git a/libcxx/include/__ranges/split_view.h b/libcxx/include/__ranges/split_view.h
index a27ac4ef7a1965..7f03be3c346a42 100644
--- a/libcxx/include/__ranges/split_view.h
+++ b/libcxx/include/__ranges/split_view.h
@@ -194,7 +194,7 @@ struct split_view<_View, _Pattern>::__sentinel {
 
 namespace views {
 namespace __split_view {
-struct __fn : __range_adaptor_closure<__fn> {
+struct __fn {
   // clang-format off
   template <class _Range, class _Pattern>
   _LIBCPP_NODISCARD_EXT _LIBCPP_HIDE_FROM_ABI
diff --git a/libcxx/test/std/ranges/range.adaptors/range.lazy.split/adaptor.pass.cpp b/libcxx/test/std/ranges/range.adaptors/range.lazy.split/adaptor.pass.cpp
index da4bd9fbbe1794..6bfa0ab487ba1b 100644
--- a/libcxx/test/std/ranges/range.adaptors/range.lazy.split/adaptor.pass.cpp
+++ b/libcxx/test/std/ranges/range.adaptors/range.lazy.split/adaptor.pass.cpp
@@ -40,10 +40,16 @@ static_assert(!std::is_invocable_v<decltype(std::views::lazy_split), SomeView, N
 static_assert(!std::is_invocable_v<decltype(std::views::lazy_split), NotAView, SomeView>);
 static_assert( std::is_invocable_v<decltype(std::views::lazy_split), SomeView, SomeView>);
 
-static_assert( CanBePiped<SomeView&,    decltype(std::views::lazy_split)>);
-static_assert( CanBePiped<char(&)[10],  decltype(std::views::lazy_split)>);
-static_assert(!CanBePiped<char(&&)[10], decltype(std::views::lazy_split)>);
-static_assert(!CanBePiped<NotAView,     decltype(std::views::lazy_split)>);
+// Regression test for #75002, views::lazy_split shouldn't be a range adaptor closure
+static_assert(!CanBePiped<SomeView&, decltype(std::views::lazy_split)>);
+static_assert(!CanBePiped<char (&)[10], decltype(std::views::lazy_split)>);
+static_assert(!CanBePiped<char (&&)[10], decltype(std::views::lazy_split)>);
+static_assert(!CanBePiped<NotAView, decltype(std::views::lazy_split)>);
+
+static_assert(CanBePiped<SomeView&, decltype(std::views::lazy_split('x'))>);
+static_assert(CanBePiped<char (&)[10], decltype(std::views::lazy_split('x'))>);
+static_assert(!CanBePiped<char (&&)[10], decltype(std::views::lazy_split('x'))>);
+static_assert(!CanBePiped<NotAView, decltype(std::views::lazy_split('x'))>);
 
 static_assert(std::same_as<decltype(std::views::lazy_split), decltype(std::ranges::views::lazy_split)>);
 
diff --git a/libcxx/test/std/ranges/range.adaptors/range.split/adaptor.pass.cpp b/libcxx/test/std/ranges/range.adaptors/range.split/adaptor.pass.cpp
index cd12011daeab5d..85d13ac5c29dfb 100644
--- a/libcxx/test/std/ranges/range.adaptors/range.split/adaptor.pass.cpp
+++ b/libcxx/test/std/ranges/range.adaptors/range.split/adaptor.pass.cpp
@@ -39,10 +39,16 @@ static_assert(!std::is_invocable_v<decltype(std::views::split), SomeView, NotAVi
 static_assert(!std::is_invocable_v<decltype(std::views::split), NotAView, SomeView>);
 static_assert( std::is_invocable_v<decltype(std::views::split), SomeView, SomeView>);
 
-static_assert( CanBePiped<SomeView&,    decltype(std::views::split)>);
-static_assert( CanBePiped<char(&)[10],  decltype(std::views::split)>);
-static_assert(!CanBePiped<char(&&)[10], decltype(std::views::split)>);
-static_assert(!CanBePiped<NotAView,     decltype(std::views::split)>);
+// Regression test for #75002, views::split shouldn't be a range adaptor closure
+static_assert(!CanBePiped<SomeView&, decltype(std::views::split)>);
+static_assert(!CanBePiped<char (&)[10], decltype(std::views::split)>);
+static_assert(!CanBePiped<char (&&)[10], decltype(std::views::split)>);
+static_assert(!CanBePiped<NotAView, decltype(std::views::split)>);
+
+static_assert(CanBePiped<SomeView&, decltype(std::views::split('x'))>);
+static_assert(CanBePiped<char (&)[10], decltype(std::views::split('x'))>);
+static_assert(!CanBePiped<char (&&)[10], decltype(std::views::split('x'))>);
+static_assert(!CanBePiped<NotAView, decltype(std::views::split('x'))>);
 
 static_assert(std::same_as<decltype(std::views::split), decltype(std::ranges::views::split)>);

var-const

Thanks!

StephanTLavavej · 2023-12-13T10:35:57Z

Checks passed with only unrelated sporadic failures/timeouts on Android, merging this now.

StephanTLavavej added 2 commits December 12, 2023 16:56

Fix product code, add regression test.

97c2cce

StephanTLavavej requested a review from a team as a code owner December 13, 2023 01:01

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Dec 13, 2023

var-const approved these changes Dec 13, 2023

View reviewed changes

StephanTLavavej mentioned this pull request Dec 13, 2023

[libc++][test] Only views::split(pattern) can be piped #74961

Closed

StephanTLavavej merged commit 8691337 into llvm:main Dec 13, 2023

StephanTLavavej deleted the stl-31-split-v2 branch December 13, 2023 10:37

StephanTLavavej mentioned this pull request Dec 15, 2023

llvm-project Mega-Update microsoft/STL#4263

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc++] `views::split` and `views::lazy_split` shouldn't be range adaptor closures#75266

[libc++] `views::split` and `views::lazy_split` shouldn't be range adaptor closures#75266
StephanTLavavej merged 2 commits intollvm:mainfrom
StephanTLavavej:stl-31-split-v2

StephanTLavavej commented Dec 13, 2023

Uh oh!

llvmbot commented Dec 13, 2023

Uh oh!

var-const left a comment

Uh oh!

StephanTLavavej commented Dec 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

StephanTLavavej commented Dec 13, 2023

Uh oh!

llvmbot commented Dec 13, 2023

Uh oh!

var-const left a comment

Choose a reason for hiding this comment

Uh oh!

StephanTLavavej commented Dec 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants