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

[Chandler Carruth]

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.


> 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 *do* think that having a
range-empowered insert and append would make this simpler.

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 strongly suspect that our codebase would be more clear if we completely
removed std::copy from it, and instead either used member insert methods or
a range based for loop to directly stream into some output-iterator-esque
API like an ostream.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140309/9fc43e93/attachment.html>