[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 aaron at aaronballman.com
Mon Mar 10 05:56:24 PDT 2014


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



More information about the llvm-commits mailing list