This is an archive of the discontinued LLVM Phabricator instance.

RFC - [ADT] Ranges pipe syntax support + SFINAE
Needs ReviewPublic

Authored by jplayer-nv on Sep 16 2022, 2:09 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

Implement the following view adapters (per the C++20 ranges spec):

  • views::iota (mostly accurate, mismatched bound type not implemented)
  • views::reverse
  • views::early_inc (not C++20, but follows the same form as other view adapters)
  • views::filter
  • views::transform
  • views::keys
  • views::values
  • views::elements
  • views::take
  • views::drop
  • views::take_while
  • views::drop_while

All above support both the functional syntax as well as the pipe syntax:

int Test[] { 1, 2, 3 };
auto Range1 = views::drop(Test, 1);
auto Range2 = Test | views::drop(1);

Pipe closures can be stored to named temporaries and applied later:

auto LT100 = [](auto X){ return X < 100; };

// Store sequence of adapters to a named variable.
auto Closure = views::take_while(LT100) | views::reverse | views::drop(1);

int Test[]{ 1, 2, 3, 4, 101, 102, 103 };

// Apply closure sequence.
dbgs() << (Test | Closure) << '\n';
// Output: { 3, 2, 1 }

Additionally, implemented certain overloads of std::ranges::to() from the C++23 spec to enable simplified range to storage conversion:

int Test[]{1, 2, 3};
auto Vec = Test | views::reverse | to<SmallVector<int>>();

There are a few other things I would like to add to the iterator_range class which I would like to discuss:

  • Templated comparison of at least one iterator_range parameter. (I would like to add a non-templated base class to iterator_range to make the SFINAE more robust).
  • operator<<() overload for iterator_range.
  • Internally, iterator_range should call adl_begin + adl_end in the container constructor.

Another fix I've applied in this patch is to wrap my callable objects in a template (Callable<T>). This enables copy assignments for any callable type. I discovered this in the TOT range adapter code while converting ranges to a std::deque on clang 10. In that implementation, there is a copy-assignment of the input iterator (which breaks TOT code).

Additionally, everywhere begin and end member functions are called, I think we should call adl_begin and adl_end to enable C-array support for the ranges code.

Diff Detail

Event Timeline

jplayer-nv created this revision.Sep 16 2022, 2:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2022, 2:09 PM
Herald added a subscriber: mgorny. · View Herald Transcript
jplayer-nv requested review of this revision.Sep 16 2022, 2:09 PM

I wrote this code for a private target (as well as for a non-LLVM compiler framework), and was wondering if some form of it would be useful upstream (I fully expect some high-level refactoring). It seems strange that there are things defined in STLExtras.h which mimic the behavior of std::ranges elements but are named differently (mapped_iterator vs transform_iterator for example).

Also I started off using functions as the 'adapters' (like what I'm seeing in TOT), but quickly found that inline constexpr variables are more versatile.

My gut reaction is that this is quite a lot of API surface area to add without current or near-future planned usage.

Any chance you're interested in proposing this as part of a patch series to at least migrate some of the existing iterator-adaptor or custom algorithms to this usage? (@kazu - maybe this'd be of some interest to you too?)

& I seem to recall some discussion about compile-time performance problems with view heavy code? Does anyone have any info on that?

kazu added a comment.Sep 18 2022, 9:49 AM

Any chance you're interested in proposing this as part of a patch series to at least migrate some of the existing iterator-adaptor or custom algorithms to this usage? (@kazu - maybe this'd be of some interest to you too?)

I have thought about the ranges library. If somebody posts patches in small chunks, I am happy to review them. Otherwise, I am inclined to wait until we can use C++20, which should happen in 3 years or so because we just upgraded to C++17. The reason for my lazy/passive approach is as follows. I do like the expressiveness of the views with the "pipe" operator |, but it's mostly syntactic sugar that doesn't significantly reduce the token count -- A | B | C v.s. C(B(A)).

I seem to recall some discussion about compile-time performance problems with view heavy code?

I see some posts on the Internet. I'll read up on that.

My gut reaction is that this is quite a lot of API surface area to add without current or near-future planned usage.

Any chance you're interested in proposing this as part of a patch series to at least migrate some of the existing iterator-adaptor or custom algorithms to this usage? (@kazu - maybe this'd be of some interest to you too?)

I'd be happy to break this down into bite-sized chunks. Is discourse the right place to iterate on the details of such a patch series? I can start with the ability to default construct and copy-assign existing iterators which carry a callable.

& I seem to recall some discussion about compile-time performance problems with view heavy code? Does anyone have any info on that?

I'm seeing complaints regarding compile-time effect of including <algorithm>, is there anything you recall reading which was more specific to usage overhead?

The reason for my lazy/passive approach is as follows. I do like the expressiveness of the views with the "pipe" operator |, but it's mostly syntactic sugar that doesn't significantly reduce the token count -- A | B | C v.s. C(B(A)).

Agreed... it's an ergonomics improvement. My eyes start glassing over with the functional syntax if you start passing parameters:

reverse(drop(take(MyRange, 2), 4))

vs

MyRange | take(2) | drop(4) | reverse

I'm happy to upstream in small bites if this is desired, or we can stop short of the pipe syntax if it's deemed a nuisance.

My gut reaction is that this is quite a lot of API surface area to add without current or near-future planned usage.

Any chance you're interested in proposing this as part of a patch series to at least migrate some of the existing iterator-adaptor or custom algorithms to this usage? (@kazu - maybe this'd be of some interest to you too?)

I'd be happy to break this down into bite-sized chunks. Is discourse the right place to iterate on the details of such a patch series? I can start with the ability to default construct and copy-assign existing iterators which carry a callable.

Yeah, Discourse is probably the place to go to build broader consensus about this novel direction.