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

[Pete Cooper]

> On Apr 7, 2015, at 3:18 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> (+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)
I’m happy to wait on seeing what the committee does.  Although with things like unique_ptr we were willing to have our own in LLVM until we moved to C++11.  I’d be happy to see us do the same with range adapters.
> 
> On Tue, Apr 7, 2015 at 3:02 PM, Pete Cooper <peter_cooper at apple.com <mailto: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… 
Ouch!  Thanks for the breakdown of the options here.  I can see why its UB, but i’d never have thought of this myself.  Its clear that what i’ve written is about the minimum required to get something to work, but not necessarily complete.
>  
> 
> 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. 
Yeah, reverse would be extremely useful.  As would iterating over 2 containers in parallel.  I think Lang said thats like Haskell’s zip.

Anyway, i’m happy to put this patch aside for now and wait to see what comes out the standards committee.  Thanks agains for the feedback.

Cheers,
Pete
>  
> 
> Cheers,
> Pete
> 
> 
> 
> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <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/20150408/7ee5b663/attachment.html>