[libc++][test] Only `views::split(pattern)` can be piped by StephanTLavavej · Pull Request #74961 · llvm/llvm-project

StephanTLavavej · 2023-12-10T03:02:26Z

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

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.

Note: The fact that the tests were previously passing indicates the existence of a bug. @cpplearner figured out what was going on, so I filed #75002 to track followup changes for libc++'s product code.

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-10T03:02:54Z

@llvm/pr-subscribers-libcxx

Author: Stephan T. Lavavej (StephanTLavavej)

Changes

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

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.

Note: The fact that the tests were previously passing may indicate the existence of a bug (or an extension?) in libc++'s product code. I looked at the machinery briefly but it was far beyond me (I barely understand microsoft/STL's ranges machinery). As this PR changes the tests to be portable, I would like to request that if changes to libc++'s product code are desired, that should be done in a followup PR (and I can file a followup issue if a reminder is desired).

<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/74961.diff

2 Files Affected:

(modified) libcxx/test/std/ranges/range.adaptors/range.lazy.split/adaptor.pass.cpp (+4-4)
(modified) libcxx/test/std/ranges/range.adaptors/range.split/adaptor.pass.cpp (+4-4)

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..58be3713263c73 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,10 @@ 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)>);
+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..4b13d1b361198f 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,10 @@ 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)>);
+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)>);

github-actions · 2023-12-10T03:04:48Z

✅ With the latest revision this PR passed the C/C++ code formatter.

cpplearner · 2023-12-10T03:34:37Z

It seems that libc++'s decltype(std::views::split) derives from __range_adaptor_closure, which provides operator|.

llvm-project/libcxx/include/__ranges/split_view.h

Lines 195 to 197 in b85f1f9

    
           namespace views { 
        
           namespace __split_view { 
        
           struct __fn : __range_adaptor_closure<__fn> {

llvm-project/libcxx/include/__ranges/range_adaptor.h

Lines 57 to 63 in b85f1f9

    
           template <class _Tp> 
        
           struct __range_adaptor_closure { 
        
               template <ranges::viewable_range _View, _RangeAdaptorClosure _Closure> 
        
                   requires same_as<_Tp, remove_cvref_t<_Closure>> && 
        
                            invocable<_Closure, _View> 
        
               [[nodiscard]] _LIBCPP_HIDE_FROM_ABI 
        
               friend constexpr decltype(auto) operator|(_View&& __view, _Closure&& __closure)

I think decltype(std::views::split) should be like decltype(std::views::take_while), which doesn't have a base class.

llvm-project/libcxx/include/__ranges/take_while_view.h

Lines 134 to 137 in b85f1f9

    
           namespace views { 
        
           namespace __take_while { 
        
           struct __fn {

StephanTLavavej · 2023-12-13T01:22:58Z

Closing this PR as it has been superseded by #75266 which has passed stage1 checks (I'll wait for stage2 and stage3 before merging it, of course).

@cpplearner

…aptor closures (#75266) Fixes #75002. Found while running libc++'s tests with MSVC's STL. This is a superset of #74961 that also fixes the product code and adds a regression test. Thanks again, @cpplearner! 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.

StephanTLavavej requested a review from a team as a code owner December 10, 2023 03:02

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

Apply clang-format from CI.

e9ffcc0

This was referenced Dec 10, 2023

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

Closed

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

Merged

StephanTLavavej closed this Dec 13, 2023

StephanTLavavej deleted the stl-24-split-can-be-piped branch December 13, 2023 01:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

[libc++][test] Only `views::split(pattern)` can be piped#74961
StephanTLavavej wants to merge 2 commits intollvm:mainfrom
StephanTLavavej:stl-24-split-can-be-piped

StephanTLavavej commented Dec 10, 2023 •

edited

Loading

Uh oh!

llvmbot commented Dec 10, 2023

Uh oh!

github-actions bot commented Dec 10, 2023 •

edited

Loading

Uh oh!

cpplearner commented Dec 10, 2023

Uh oh!

StephanTLavavej commented Dec 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

StephanTLavavej commented Dec 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Dec 10, 2023

Uh oh!

github-actions bot commented Dec 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpplearner commented Dec 10, 2023

Uh oh!

StephanTLavavej commented Dec 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

StephanTLavavej commented Dec 10, 2023 •

edited

Loading

github-actions bot commented Dec 10, 2023 •

edited

Loading