<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jan 13, 2015 at 10:26 AM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
> On 2015-Jan-12, at 22:19, Nick Lewycky <<a href="mailto:nicholas@mxc.ca">nicholas@mxc.ca</a>> wrote:<br>
><br>
> Duncan P. N. Exon Smith wrote:<br>
>><br>
>>> On 2015-Jan-07, at 14:41, Ramkumar Ramachandra<<a href="mailto:artagnon@gmail.com">artagnon@gmail.com</a>>  wrote:<br>
>>><br>
>>> It depends on what you want to construct from the iteration. You could construct a BasicBlock, like you said; but this is perfectly valid too: you can try it out, and see that tests pass.<br>
>>><br>
>><br>
>> This works because of an implementation quirk of `ilist_iterator`,<br>
>> which can be constructed from a reference of the node type.<br>
><br>
> That's really not an implementation quirk, that's part of the intended interface and there is code which does this on purpose (admittedly rare). It's one of the nice things you can do with intrusive lists, you can always go from one node to the prev/next node. This is how we expose that.<br>
<br>
</span>Right, that's how intrusive lists work; but IMO implicitly<br>
converting to iterators is a dangerous API leak (made all the more<br>
dangerous by range-based `for`s, where code like this "looks" like<br>
it's doing something it isn't: `bypassSlowDivision()` is expecting<br>
to advance the loop iterator, but it's not).<br></blockquote><div><br></div><div>I agree. It's incredibly confusing for a range-based for to have the iterator as the declared variable. And as programmers we must avoid confusion.</div><div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I think this should be an `explicit` constructor, like in the<br>
attached patch (which changes the API and updates a few (of the<br>
many) call sites).<br>
<br>
<br><br>
<br>
><br>
>> While technically valid, this code is misleading, and I don't know<br>
>> that we want to propagate dependencies on that conversion.  I think<br>
>> this should instead be, e.g.:<br>
>><br>
>>     for (BasicBlock&BB : F)<br>
>>       EverMadeChange |= bypassSlowDivision(F,&BB, BypassWidths);<br>
>><br>
>> Thoughts from anyone else?<br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@cs.uiuc.edu">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>
><br>
<br>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">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>