<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Apr 8, 2015 at 11:05 AM, Pete Cooper <span dir="ltr"><<a href="mailto:peter_cooper@apple.com" target="_blank">peter_cooper@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><span class=""><blockquote type="cite"><div>On Apr 7, 2015, at 3:18 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><div dir="ltr">(+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)<br></div></div></blockquote></span>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.</div></div></blockquote><div><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>  I’d be happy to see us do the same with range adapters.<br></div></div></blockquote><div><br>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)) </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Tue, Apr 7, 2015 at 3:02 PM, Pete Cooper <span dir="ltr"><<a href="mailto:peter_cooper@apple.com" target="_blank">peter_cooper@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi all<br>
<br>
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.<br></blockquote><div><br>Usual caveat for range adapters (not a problem for /iterator/ adapters) is lifetime:<br><br>container func();<br><br>for (auto x : func()) // this is fine<br>  f(x);<br><br>for (auto x : wrap(func()) // this will invoke UB</div></span><div><span class="">  f(x)<br><br>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. <br><br>This is the main thing holding up standardization of range adapters in C++, as I understand it (though I haven't followed closely).<br><br>Options include<br><br>1) disallow: add an overload of IndexedIterator that takes the container by rvalue reference, and = delete it<br>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)<br></span>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… <br></div></div></div></div></div></blockquote>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.</div></div></blockquote><div><br>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)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><span class=""><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I’ve updated a loop in AArch64A57FPLoadBalancing to show this.  The gist of the change is that the loop goes from<br>
<br>
  unsigned Idx = 0;<br>
  for (auto &MI : MBB)<br>
    scanInstruction(&MI, Idx++, ActiveChains, AllChains);<br>
<br>
to<br>
<br>
  for (auto MI : IndexedIterator(MBB))<br>
    scanInstruction(&*MI, MI.index(), ActiveChains, AllChains);<br>
<br>
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).<br></blockquote><div><br>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... <br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.<br></blockquote><div><br>Yeah, there's a bunch of range adapters that would be handy. Lang & I were talking about the desire for a "reverse" adapter. <br></div></div></div></div></div></blockquote></span>Yeah, reverse would be extremely useful.  As would iterating over 2 containers in parallel.  I think Lang said thats like Haskell’s zip.</div></div></blockquote><div><br>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)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>Anyway, i’m happy to put this patch aside for now and wait to see what comes out the standards committee. </div></div></blockquote><div><br>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.<br><br>Just some thoughts,<br><br>- David<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div> Thanks agains for the feedback.<br></div><div><br></div><div>Cheers,</div><div>Pete<span class=""><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Cheers,<br>
Pete<br>
<br>
<br><br>
<br>
<br>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div></div>
</div></blockquote></span></div><br></div></blockquote></div><br></div></div>