[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
Fri Mar 14 05:07:08 PDT 2014


On Fri, Mar 14, 2014 at 6:41 AM, Alexey Samsonov <samsonov at google.com> wrote:
>
> On Mon, Mar 10, 2014 at 5:11 AM, 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.
>
>
> This question may be naive, but... If not distance(), can we make use of
>   bool empty() const { return begin_iterator == end_iterator; }
> I sometimes see the code like:
>   if (S.relocation_begin() != S.relocation_end()) {
>      ///
>   }
> it may be nice to rewrite this as:
>   if (!S.relocations().empty()) { ... }

That's a pattern I am seeing relatively often in the frontend, but
it's on S and not the range. I think an empty range does make sense
theoretically, but whether it's better to say S.relocations().empty()
or S.relocations_empty() is a bit ambiguous.

~Aaron



More information about the llvm-commits mailing list