[PATCH] CodeGenPrep: rewrite a few loops in C++11 style
chisophugis at gmail.com
Tue Jan 13 13:06:12 PST 2015
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>
> >>> 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
-- 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
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the llvm-commits