[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]

Committed the LLVM side of things in r203460. The corresponding Clang
changes are in r203461.

~Aaron

On Mon, Mar 10, 2014 at 8:56 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
> On Sun, Mar 9, 2014 at 9:11 PM, Chandler Carruth <chandlerc at google.com> wrote:
>> On Sun, Mar 9, 2014 at 5:51 AM, Aaron Ballman <aaron at aaronballman.com>
>> wrote:
>>>
>>> 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 think that if we need this, STLExtras is probably the right place.
>> However, the more I think about it, the less I think these utiltiies are
>> good.
>>
>>>
>>> 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.
>>
>>
>> After thinking about it more, I really don't agree. See below.
>>
>>>
>>>
>>> >
>>> >>
>>> >> 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.
>>
>>
>> The more I think about it, the more strongly I disagree.
>>
>> First off, the name makes no sense. There is no "distance" for a range. The
>> word "distance" implies two points, and thus begin and end. I think the code
>> is considerably cleaner getting begin and end and computing the distance
>> between them.
>>
>> It also ensures that the intended semantic difference between distance and a
>> size method is conveyed -- if the two points have no direct distance
>> relationship, the computation may be linear. I think that the code with
>> iterators makes this more clear and is not nearly common enough to be an
>> actual burden to type out. Let's go back to just the iterator variant.
>
> I agree with your reasoning; I'll take care of this.
>
>>
>>>
>>> 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.
>>
>>
>> Here too, I find there to be benefit.
>>
>> First, std::copy is ultimately a bit of a hack to deal with output iterators
>> and such. Its uses are very rare. I suspect that most of our uses today
>> could be better written as <foo>.insert(<foo>end(), Records->field_begin(),
>> Records->field_end()); I think this is more clear than std::copy by a large
>> margin.
>
> I would say that at best, it's as clear as copy. I don't think there's
> a definite "winner" between the two, but take your point.
>
>> I *do* think that having a range-empowered insert and append would
>> make this simpler.
>
> As well as constructors, I would assume?
>
>> With std::copy, using a range interface opens really serious semantic
>> questions IMO. Many algorithms are going to (I hope) have more value
>> semantics rather than mutating in-place. Thus they will return results
>> rather than accepting output iterators. But that doesn't make any sense for
>> copy because copy doesn't make any sense in a value-semantic world -- it
>> would just be assignment.
>>
>> So here too, I think we should just go back to the iterator variant. Trying
>> to define clear and unambiguous APIs to save a *very* small amount of typing
>> (and no complexity really) is just not worth it, especially for APIs as
>> strange and contorted as std::copy.
>
> I don't agree as strongly as I do with std::distance, but I also don't
> disagree particularly strongly either. I'll remove these changes, and
> replace the copies with a range-based constructor or a range-based
> insert as appropriate.
>
> Thank you for the review and feedback!
>
> ~Aaron