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


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

 /// \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.


> +
>    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?

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.

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.


Also, as a meta style point: please prefer "typename" to "class"
everywhere. Please prefer "FooT" to "Foo" for parameters everywhere.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140308/659f976b/attachment.html>