[PATCH] Add a wrapper to iterator to return the index of the iterator

[David Blaikie]

(+Chandler because I've talked to him a few times about range adapters and
he's expressed hesitancy, I believe hoping to wait for some direction to
come out of the C++ standards committee, but that was quite a while back -
not sure what the state is now)

On Tue, Apr 7, 2015 at 3:02 PM, Pete Cooper <peter_cooper at apple.com> wrote:

> Hi all
>
> This patch adds a wrapper to iterators to allow us to use foreach loops,
> but also get the index of elements.  This allows a greater number of loops
> to use foreach, or avoid a foreach with a separate index.
>

Usual caveat for range adapters (not a problem for /iterator/ adapters) is
lifetime:

container func();

for (auto x : func()) // this is fine
  f(x);

for (auto x : wrap(func()) // this will invoke UB
  f(x)

the problem being that range-for uses reference-extension to extend the
lifetime of the container, but if it's wrapped then only the lifetime of
the wrapper will be extended.

This is the main thing holding up standardization of range adapters in C++,
as I understand it (though I haven't followed closely).

Options include

1) disallow: add an overload of IndexedIterator that takes the container by
rvalue reference, and = delete it
2) do nothing (the range adapter would still work for temporaries when
passed to another function that does the work, rather than to a range-for
loop or other reference-extension context:
do_things(reverse(createSequence())); for example)
3) get really fancy and have range adapter that takes rvalue reference
objects and moves them into the adapter itself to extend their lifetime -
there's all sorts of interesting gotchas here, especially once you get
around to composability of range adapters...


>
> I’ve updated a loop in AArch64A57FPLoadBalancing to show this.  The gist
> of the change is that the loop goes from
>
>   unsigned Idx = 0;
>   for (auto &MI : MBB)
>     scanInstruction(&MI, Idx++, ActiveChains, AllChains);
>
> to
>
>   for (auto MI : IndexedIterator(MBB))
>     scanInstruction(&*MI, MI.index(), ActiveChains, AllChains);
>
> So accesses to the iterator need a *, and there’s a new index() method.  I
> considered adding a .value() to the iterator instead of *.  I’m not
> strongly attached to one over the other (or both).
>

Yeah, I'd probably make it a trivial struct with a "index" and "value"
member (the value member would be a const ref - though this makes the
"auto" versus "auto&" in the range-for a bit misleading...


> Comments welcome.  I personally like the idea wrapping iterators, but i
> understand if anyone is fundamentally opposed.  I’d really like to add
> another wrapper later which keeps track of the next pointer for you as then
> the current element can be safely deleted.
>

Yeah, there's a bunch of range adapters that would be handy. Lang & I were
talking about the desire for a "reverse" adapter.


>
> Cheers,
> Pete
>
>
>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150407/5451b563/attachment.html>