[PATCH] CodeGenPrep: rewrite a few loops in C++11 style

[Sean Silva]

On Tue, Jan 13, 2015 at 10:26 AM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > On 2015-Jan-12, at 22:19, Nick Lewycky <nicholas at mxc.ca> wrote:
> >
> > Duncan P. N. Exon Smith wrote:
> >>
> >>> On 2015-Jan-07, at 14:41, Ramkumar Ramachandra<artagnon at gmail.com>
> wrote:
> >>>
> >>> 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.
> >>>
> >>
> >> This works because of an implementation quirk of `ilist_iterator`,
> >> which can be constructed from a reference of the node type.
> >
> > 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.
>
> Right, that's how intrusive lists work; but IMO implicitly
> converting to iterators is a dangerous API leak (made all the more
> dangerous by range-based `for`s, where code like this "looks" like
> it's doing something it isn't: `bypassSlowDivision()` is expecting
> to advance the loop iterator, but it's not).
>

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.

-- Sean Silva


>
> I think this should be an `explicit` constructor, like in the
> attached patch (which changes the API and updates a few (of the
> many) call sites).
>
>
>
>
> >
> >> While technically valid, this code is misleading, and I don't know
> >> that we want to propagate dependencies on that conversion.  I think
> >> this should instead be, e.g.:
> >>
> >>     for (BasicBlock&BB : F)
> >>       EverMadeChange |= bypassSlowDivision(F,&BB, BypassWidths);
> >>
> >> Thoughts from anyone else?
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >>
> >
>
>
> _______________________________________________
> 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/20150113/8b0774f9/attachment.html>