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

Thu Apr 9 10:45:40 PDT 2015

> On Apr 8, 2015, at 7:49 PM, Chandler Carruth <chandlerc at google.com> wrote:
> 
> FYI, my opinion about these issues has definitely shifted since we last discussed this David... I'm increasingly interested in us just charging forward on this front.
Charging forward sounds good to me :)
> 
> Currently, I'd like to think a bit more about what pattern we should follow for two aspects of this:
> 
> - How do we pack N things that are being iterated over together? This iterator and zip need an answer here.
Yeah, thinking about it more i’m not even sure if my solution was right to store a reference to an element, and not the original iterator inside it.  Thats one of the questions I guess we need to answer.  

I did also originally try to use the llvm iterator_adaptor_base with the theory that my iterator just converts an iterator to an indexed one, and we can just store in the index in the wrapper, but i never managed to get it to compile all the tests in the patch.
> - How do we handle temporaries as Dave points out? This is also not a trivial tradeoff. :: sigh ::
Yeah, i totally missed those when I wrote my code.  I think Dave’s right that we don’t necessarily have to solve that 100% to make progress here, but probably will need to eventually.  I’d be fine to start with a solution which at least uses enable_if to ban temporaries in wrapped iterators if one is possible.
> 
> For both, its not that I think any of the options are *bad*, it's that I think it will be hard to start off conservatively and then try to be more aggressive (because the payoff will be too low) and I think we will want to strive for consistency across a number of range adaptors.
Hmm, well this goes against me saying we don’t need a 100% solution.  I get your point though.  I’m not opposed to the 100% solution either if that works :)
> 
> So I'd like to revisit this post EuroLLVM if that's OK? Hoping I can squeeze in some time while traveling to play with this and other things and see how things are looking.
Yep, sounds good.

...
> 
> -Chandler
> 
> On Wed, Apr 8, 2015 at 11:23 AM David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
> On Wed, Apr 8, 2015 at 11:05 AM, Pete Cooper <peter_cooper at apple.com <mailto:peter_cooper at apple.com>> wrote:
> 
>> On Apr 7, 2015, at 3:18 PM, David Blaikie <dblaikie at gmail.com <mailto: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.
> 
> There wasn't too much to wait on there, make_unique was already a shoe-in for 14 by the time we were adopting 11.
>  
>  I’d be happy to see us do the same with range adapters.
> 
> Perhaps my pessimism was more than I intended - I'm actually in favor of range adapters and I think it's somewhat limiting to keep pushing back on this (& CC Chandler to get a current gauge of his perspective, since the last time I spoke to him about this was quite a while back and things may've changed (& I've probably misrememebred the specifics in the interim anyway)) 
Oh no.  I didn’t take it a pessimism, just that we should have a good discussion about this and how to do it right.  And i’m totally fine with that.

Cheers,
Pete
>> 
>> 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'm actually personally OK with (1) or even (2) (can do (1) until someone hits the "but I can totally use a temporary because I'm using the adapter only within this full expression" case I demonstrated above). Within a smaller codebase like LLVM (rather than "all C++ code forever" that the C++ standards committee has to deal with) I'm willing to be a bit more cavalier. (& tools like MSan, etc, can help us of course)
>  
> 
>>  
>> 
>> 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.
> 
> Yep - that's another (& indeed we could have a generating range that just produces numbers, then zip that together with the original range to create your indexing range)
>  
> Anyway, i’m happy to put this patch aside for now and wait to see what comes out the standards committee.
> 
> It's an option, but I wouldn't personally mind not waiting and trying a few things ourselves. Given how terse these sort of things make code, I wouldn't imagine any good solution to come out of the committee would be hard to transform our code to conform to. There are limited options that have the readability/convenience that is the point of these things.
> 
> Just some thoughts,
> 
> - David
>  
>  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>
>> 
>> 
> 
> _______________________________________________
> 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/20150409/59948661/attachment.html>