[llvm] r203354 - Adding range-based STL-like helper APIs. llvm::distance() is the range version of std::distance. llvm::copy is the range version of std::copy.

[Aaron Ballman]

On Sat, Mar 8, 2014 at 8:23 PM, Chandler Carruth <chandlerc at google.com> wrote:
> On Sat, Mar 8, 2014 at 12:11 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> Author: aaronballman
>> Date: Sat Mar  8 14:11:24 2014
>> New Revision: 203354
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=203354&view=rev
>> Log:
>> Adding range-based STL-like helper APIs. llvm::distance() is the range
>> version of std::distance. llvm::copy is the range version of std::copy.
>>
>> Modified:
>>     llvm/trunk/include/llvm/ADT/iterator_range.h
>
>
> I don't think these belong in this header at all.

I was going to put them into STLExtras.h, except that didn't seem
right given that ranges aren't in the STL yet. Do you have a better
suggestion as to where they should live? RangeExtras.h perhaps?

I'm only adding things which come up several places. I don't mind
using iterators if it's one- or two-off instances, but when it's 4-5+,
it seems beneficial to have range versions of some iterator APIs.

>
>>
>> Modified: llvm/trunk/include/llvm/ADT/iterator_range.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/iterator_range.h?rev=203354&r1=203353&r2=203354&view=diff
>>
>> ==============================================================================
>> --- llvm/trunk/include/llvm/ADT/iterator_range.h (original)
>> +++ llvm/trunk/include/llvm/ADT/iterator_range.h Sat Mar  8 14:11:24 2014
>> @@ -23,6 +23,11 @@
>>
>>  namespace llvm {
>>
>> +template <typename Range>
>> +struct range_traits {
>> +  typedef typename Range::difference_type difference_type;
>> +};
>> +
>
>
> There is no documentation for this. We have no pattern or reference from the
> standard about how it should work. I don't even know where you're really
> going here or why we need this at all. It feels like just boilerplate to me.

I was modelling off the MSVC iterator implementation for how
std::distance was implemented. But I'm fine yanking the traits.

>
>>  /// \brief A range adaptor for a pair of iterators.
>>  ///
>>  /// This just wraps two iterators into a range-compatible interface.
>> Nothing
>> @@ -32,6 +37,10 @@ class iterator_range {
>>    IteratorT begin_iterator, end_iterator;
>>
>>  public:
>> +  // FIXME: We should be using iterator_traits to determine the
>> +  // difference_type, but most of our iterators do not expose anything
>> like it.
>> +  typedef int difference_type;
>
>
> I don't think we should expose this until those iterators are fixed and we
> can use something sensible. "int" doesn't really seem like a sensible choice
> for most of our iterators.

Probably should be ptrdiff_t, in retrospect. But if the boilerplate is
going away, no need to expose this.

>
>>
>> +
>>    iterator_range() {}
>>    iterator_range(IteratorT begin_iterator, IteratorT end_iterator)
>>        : begin_iterator(std::move(begin_iterator)),
>> @@ -41,6 +50,19 @@ public:
>>    IteratorT end() const { return end_iterator; }
>>  };
>>
>> +/// \brief Determine the distance between the end() and begin() iterators
>> of
>> +/// a range. Analogous to std::distance().
>> +template <class Range>
>> +typename range_traits<Range>::difference_type distance(Range R) {
>> +  return std::distance(R.begin(), R.end());
>> +}
>
>
> Is this really worth providing at this point?

Yes, it gets used about a half-dozen times at least, and it looks
considerably easier on the eye using the range API.

> If it is, why not define it with the return type computed from
> std::distance's return type? I don't see a need for traits yet.

Something along the lines of:

template <typename RangeT>
auto distance(RangeT R) {
  return std::distance(R.begin(), R.end());
}

?

>
> It doesn't belong in this header file either way.
>
>> +
>> +/// \brief Copies members of a range into the output iterator provided.
>> +/// Analogous to std::copy.
>> +template <class Range, class OutputIterator>
>> +OutputIterator copy(Range In, OutputIterator Result) {
>> +  return std::copy(In.begin(), In.end(), Result);
>> +}
>
>
> Same problems. This provides no actual advantage over just using the
> iterator form. I would just use the iterator form. If we do provide it, it
> shouldn't be in this header.

The iterator form is more boilerplate typing without benefit:

std::copy(Records->field_begin(), Records->field_end(),
std::back_inserter(Fields));

vs

llvm::copy(Records->fields(), std::back_inserter(Fields));

So I'd disagree that it provides no actual benefit, but would agree
that it's not drastic benefit.

> Also, as a meta style point: please prefer "typename" to "class" everywhere.
> Please prefer "FooT" to "Foo" for parameters everywhere.

Can do!

~Aaron