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

[David Blaikie]

On Wed, Apr 8, 2015 at 11:05 AM, Pete Cooper <peter_cooper at apple.com> wrote:

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

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

>
> 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…
>
> 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
>> 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/2de3b7b9/attachment.html>