<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Feb 9, 2017 at 9:23 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><span class=""><div dir="ltr">On Thu, Feb 9, 2017 at 8:14 AM Daniel Berlin via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">dberlin created this revision.<br class="m_7542136315290529828gmail_msg">
Herald added a reviewer: tstellarAMD.<br class="m_7542136315290529828gmail_msg">
Herald added subscribers: nhaehnle, arsenm.<br class="m_7542136315290529828gmail_msg">
<br class="m_7542136315290529828gmail_msg">
Convert all obvious node_begin/node_end and child_begin/child_end<br class="m_7542136315290529828gmail_msg">
pairs to range based for.<br class="m_7542136315290529828gmail_msg">
<br class="m_7542136315290529828gmail_msg">
Sending for review in case someone has a good idea how to make<br class="m_7542136315290529828gmail_msg">
graph_children able to be inferred. It looks like it would require<br class="m_7542136315290529828gmail_msg">
changing GraphTraits to be two argument or something. I presume<br class="m_7542136315290529828gmail_msg">
inference does not happen because it would have to check every<br class="m_7542136315290529828gmail_msg">
GraphTraits in the world to see if the noderef types matched.<br class="m_7542136315290529828gmail_msg"></blockquote><div><br></div></span><div>Yep, that. One could use an extra trait for the node type to map it back to the graph trait to facilitate this - but that'd also mean each node type would have to be unique to the graph type (which is probably true - but one could imagine different graphs over the same nodes? Maybe... maybe not)<br></div></div></div></blockquote><div><br></div><div><br></div><div>We have different graphs over the same nodes already.</div><div><br></div><div>The nodes of a graph of the function *are basicblock *</div><div>the nodes of a graph of basicblock * are also basicblock *</div><div><br></div><div>But they are not the same graph :)<br><br></div><div>This only currently works through chicanery, IMHO. It would be nice to be able to define different graphs over the same types, but i'm not up for rewriting all of graph traits ATM.</div><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br>Did you consider changing the trait so they exposed range accessors directly, instead of adding utility functions?<br></div></div></div></blockquote><div><br></div><div>I can't figure out a way to do this.</div><div><br></div><div>The base class traits have *nothing* in them.</div><div> </div><div>This was the best i could do without "rewrite graphtraits completely".</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div>"graph_children(graph)" is a bit of an awkward construct </div></div></div></blockquote><div><br></div><div>Note that if i put it in the traits, i believe this would get worse, not matter.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div>. Alternatively maybe drop the graph_ prefix - ADL/overload resolution/etc will make it correct anyway, so just "children(graph)", etc?<br></div></div></div></blockquote><div><br></div><div>I'm happy to drop the prefix.</div><div> <br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div>(re: your other comment, I'm not sure I quite followed:<br><br>"<span style="color:rgb(33,33,33);font-size:13px">Note that this use of const auto & is required to make clang build, as the iterator type it for CFG blocks ends up returning blocks directly otherwise here." - not sure I quite follow. Best guess/reading is that the CFG's iterators' value type is heavyweight, but most other GraphTraits types have lightweight nodes that are cheap to copy? (like pointers, etc)<br></span></div></div></div></blockquote><div><br></div><div>Sorta.</div><div><br></div><div>First, graph traits doesn't work on anything but pointers.  Which is very very sad, but also outside the scope of this patch (and also means you can't, for example, have a graph of pairs or wahtever).  Some day i will fix this.</div><div><br></div><div>In any case, this particular issue is:</div><div><br></div><div>the child iterators are expected to return noderefs, but it looks like the iterator for clang's CFG returns a value type.<br></div><div><br></div><div>IE the iterators return, say, BasicBlock * (a pointer type).</div><div>Clang's returns AdjacentCFGBlock  (a value type).</div><div><br></div><div>Thus, if i use "auto *" instead of "const auto &", it fails.</div><div><br></div><div>I believe clang is broken here, but honestly, it's hard to tell.</div><div><br></div><div><br></div></div></div></div>