Add llvm::enumerate() to stl extras
Mehdi Amini via llvm-commits
llvm-commits at lists.llvm.org
Thu Sep 29 15:48:13 PDT 2016
> On Sep 29, 2016, at 2:03 PM, Zachary Turner via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>
> Here's the latest iteration. Added doxygen comments and removed postfix ++ operator and operator==. Also removed the overload of llvm::enumerate() and used a single function that takes an rvalue reference instead of overloading on const Range& and Range&
>
> From ae8f3cb0a9d1958b4c2dd3fcff84e22cd9bb4ba9 Mon Sep 17 00:00:00 2001
> From: Zachary Turner <zturner at google.com <mailto:zturner at google.com>>
> Date: Thu, 29 Sep 2016 11:13:38 -0700
> Subject: [PATCH] Add llvm::enumerate to STLExtras.
>
> ---
> include/llvm/ADT/STLExtras.h | 54 +++++++++++++++++++++++++++++++++++++++++
> unittests/ADT/STLExtrasTest.cpp | 42 ++++++++++++++++++++++++++++++++
> 2 files changed, 96 insertions(+)
>
> diff --git a/include/llvm/ADT/STLExtras.h b/include/llvm/ADT/STLExtras.h
> index e6215e4..63f919d 100644
> --- a/include/llvm/ADT/STLExtras.h
> +++ b/include/llvm/ADT/STLExtras.h
> @@ -626,6 +626,60 @@ template <typename T> struct deref {
> }
> };
>
> +namespace detail {
> +template <typename I, typename V> class enumerator_impl {
> +public:
> + template <typename V> struct result_pair {
> + result_pair(std::size_t Index, V Value) : Index(Index), Value(Value) {}
> +
> + const std::size_t Index;
> + V Value;
> + };
> +
> + template <typename I, typename V> struct iterator {
> + iterator(I Iter, std::size_t Index) : Iter(Iter), Index(Index) {}
> +
> + result_pair<const V> operator*() const {
> + return result_pair<const V>(Index, *Iter);
> + }
> + result_pair<V> operator*() { return result_pair<V>(Index, *Iter); }
> +
> + iterator &operator++() {
> + ++Iter;
> + ++Index;
> + return *this;
> + }
> +
> + bool operator!=(const iterator &RHS) const { return Iter != RHS.Iter; }
> +
> + private:
> + I Iter;
> + std::size_t Index;
> + };
> +
> + explicit enumerator_impl(I Begin, I End) : Begin(Begin), End(End) {}
I wonder if some iterators can be non-copyable?
Maybe move here?
> +
> + iterator<I, V> begin() { return iterator<I, V>(Begin, 0); }
> + iterator<I, V> end() { return iterator<I, V>(End, std::size_t(-1)); }
> +
> + iterator<I, V> begin() const { return iterator<I, V>(Begin, 0); }
> + iterator<I, V> end() const { return iterator<I, V>(End, std::size_t(-1)); }
> +
> +private:
> + I Begin;
> + I End;
> +};
> +}
> +
> +/// Given an input range, returns a new range whose values are are pair (A,B)
> +/// such that A is the 0-based index of the item in the sequence, and B is
> +/// the value from the original sequence.
I think it’d be great to have a snippet of pseudo-code included here.
> +template <typename R> auto enumerate(R &&Range) {
> + typedef decltype(std::begin(Range)) I;
> + typedef decltype(*std::begin(Range)) V;
> + return detail::enumerator_impl<I, V>(std::begin(Range), std::end(Range));
> +}
> +
> } // End llvm namespace
>
> #endif
> diff --git a/unittests/ADT/STLExtrasTest.cpp b/unittests/ADT/STLExtrasTest.cpp
> index dc62b03..d7457f5 100644
> --- a/unittests/ADT/STLExtrasTest.cpp
> +++ b/unittests/ADT/STLExtrasTest.cpp
> @@ -10,6 +10,8 @@
> #include "llvm/ADT/STLExtras.h"
> #include "gtest/gtest.h"
>
> +#include <vector>
> +
> using namespace llvm;
>
> namespace {
> @@ -37,4 +39,44 @@ TEST(STLExtrasTest, Rank) {
> EXPECT_EQ(4, f(rank<6>()));
> }
>
> +TEST(STLExtrasTest, Enumerate) {
> + std::vector<char> foo = {'a', 'b', 'c'};
> +
> + std::vector<std::pair<std::size_t, char>> results;
> +
> + for (auto X : llvm::enumerate(foo)) {
> + results.push_back(std::make_pair(X.Index, X.Value));
> + }
> + ASSERT_EQ(3, results.size());
> + EXPECT_EQ(0, results[0].first);
> + EXPECT_EQ('a', results[0].second);
> + EXPECT_EQ(1, results[1].first);
> + EXPECT_EQ('b', results[1].second);
> + EXPECT_EQ(2, results[2].first);
> + EXPECT_EQ('c', results[2].second);
> +
> + results.clear();
> + const std::vector<int> bar = {'1', '2', '3'};
> + for (auto X : llvm::enumerate(bar)) {
> + results.push_back(std::make_pair(X.Index, X.Value));
> + }
> + EXPECT_EQ(0, results[0].first);
> + EXPECT_EQ('1', results[0].second);
> + EXPECT_EQ(1, results[1].first);
> + EXPECT_EQ('2', results[1].second);
> + EXPECT_EQ(2, results[2].first);
> + EXPECT_EQ('3', results[2].second);
> +}
> +
> +TEST(STLExtrasTest, EnumerateModify) {
> + std::vector<char> foo = {'a', 'b', 'c'};
> +
> + for (auto X : llvm::enumerate(foo)) {
> + ++X.Value;
> + }
> +
> + EXPECT_EQ('b', foo[0]);
> + EXPECT_EQ('c', foo[1]);
> + EXPECT_EQ('d', foo[2]);
> +}
> }
Can you add a test with an empty range?
Otherwise, LGTM.
Thanks,
—
Mehdi
>
> On Thu, Sep 29, 2016 at 1:33 PM Adrian McCarthy <amccarth at google.com <mailto:amccarth at google.com>> wrote:
> I'm looking at http://en.cppreference.com/w/cpp/language/range-for <http://en.cppreference.com/w/cpp/language/range-for>, which suggests that the range-based for is going to call begin and end on your range to get the iterators, and then use operator!= and operator++ (prefix) on the iterators.
>
> Since you're testing only with range-based for loops, perhaps the unnecessary bits should be cut (like operator== and operator++(int) (postfix)). Either that or test those explicitly.
>
> On Thu, Sep 29, 2016 at 1:11 PM, Zachary Turner <zturner at google.com <mailto:zturner at google.com>> wrote:
> I don't think so, but someone correct me if I'm wrong. enumerator works with ranges, not iterators. So by extension enumerate should also be considered to return a range, and not an iterator. The baggage associated with ranges are much less than that associated with iterators. As long as you have a begin and an end you're good to go.
>
> But then again who knows, someone with more knowledge than me will have to chime in :)
>
> On Thu, Sep 29, 2016 at 1:06 PM Adrian McCarthy <amccarth at google.com <mailto:amccarth at google.com>> wrote:
> Should `enumerator_impl::iterator` have the extra baggage for `std::iterator_traits`, like typedefs for `value_type`, `iterator_category`, and such?
>
> On the other hand, if this is strictly for range-based for loops, then perhaps you don't even need the post-fix increment.
>
>
> On Thu, Sep 29, 2016 at 12:34 PM, Zachary Turner via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
> Yes, you beat me to it. I just used container in my example, but the implementation is supposed to work with any range
>
> On Thu, Sep 29, 2016 at 12:33 PM Krzysztof Parzyszek via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
> On 9/29/2016 2:27 PM, Krzysztof Parzyszek via llvm-commits wrote:
> >
> > How about enumerate(range) instead of container?
>
> Nevermind. It's the same thing.
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160929/76095279/attachment-0001.html>
More information about the llvm-commits
mailing list