<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Thu, Feb 9, 2017 at 9:37 AM Daniel Berlin <<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg">On Thu, Feb 9, 2017 at 9:23 AM, David Blaikie <span dir="ltr" class="gmail_msg"><<a href="mailto:dblaikie@gmail.com" class="gmail_msg" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br class="gmail_msg"><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg"><br class="gmail_msg"><br class="gmail_msg"><div class="gmail_quote gmail_msg"><span class="gmail_msg"><div dir="ltr" class="gmail_msg">On Thu, Feb 9, 2017 at 8:14 AM Daniel Berlin via Phabricator <<a href="mailto:reviews@reviews.llvm.org" class="gmail_msg" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br class="gmail_msg"></div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">dberlin created this revision.<br class="m_3897984082116298221m_7542136315290529828gmail_msg gmail_msg">
Herald added a reviewer: tstellarAMD.<br class="m_3897984082116298221m_7542136315290529828gmail_msg gmail_msg">
Herald added subscribers: nhaehnle, arsenm.<br class="m_3897984082116298221m_7542136315290529828gmail_msg gmail_msg">
<br class="m_3897984082116298221m_7542136315290529828gmail_msg gmail_msg">
Convert all obvious node_begin/node_end and child_begin/child_end<br class="m_3897984082116298221m_7542136315290529828gmail_msg gmail_msg">
pairs to range based for.<br class="m_3897984082116298221m_7542136315290529828gmail_msg gmail_msg">
<br class="m_3897984082116298221m_7542136315290529828gmail_msg gmail_msg">
Sending for review in case someone has a good idea how to make<br class="m_3897984082116298221m_7542136315290529828gmail_msg gmail_msg">
graph_children able to be inferred. It looks like it would require<br class="m_3897984082116298221m_7542136315290529828gmail_msg gmail_msg">
changing GraphTraits to be two argument or something. I presume<br class="m_3897984082116298221m_7542136315290529828gmail_msg gmail_msg">
inference does not happen because it would have to check every<br class="m_3897984082116298221m_7542136315290529828gmail_msg gmail_msg">
GraphTraits in the world to see if the noderef types matched.<br class="m_3897984082116298221m_7542136315290529828gmail_msg gmail_msg"></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></span><div class="gmail_msg">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 class="gmail_msg"></div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg"><br class="gmail_msg"></div></div></div></div><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">We have different graphs over the same nodes already.</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">The nodes of a graph of the function *are basicblock *</div><div class="gmail_msg">the nodes of a graph of basicblock * are also basicblock *</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">But they are not the same graph :)<br class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">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></div></div><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg"> <br class="gmail_msg"></div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"><br class="gmail_msg">Did you consider changing the trait so they exposed range accessors directly, instead of adding utility functions?<br class="gmail_msg"></div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div></div><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">I can't figure out a way to do this.</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">The base class traits have *nothing* in them.</div><div class="gmail_msg"> </div><div class="gmail_msg">This was the best i could do without "rewrite graphtraits completely".</div></div></div></div><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">"graph_children(graph)" is a bit of an awkward construct </div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div></div><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">Note that if i put it in the traits, i believe this would get worse, not matter.</div></div></div></div></blockquote><div><br></div><div>How so? Worse in terms of implementation complexity, or user complexity?<br><br>I imagine it would be worse for implementation complexity if it was added to traits in a backwards compatible way (ie: where all exisitng traits didn't have to change their API and would continue vending begins and ends, rather than ranges). If all the traits had to change then I'm not sure of the worsening that might cause.<br><br>(I suppose the inverse cases would remain non-members, creating an undesirable asymmetry perhaps?)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">. Alternatively maybe drop the graph_ prefix - ADL/overload resolution/etc will make it correct anyway, so just "children(graph)", etc?<br class="gmail_msg"></div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div></div><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">I'm happy to drop the prefix.</div></div></div></div><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"> <br class="gmail_msg"></div><div class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">(re: your other comment, I'm not sure I quite followed:<br class="gmail_msg"><br class="gmail_msg">"<span style="color:rgb(33,33,33);font-size:13px" class="gmail_msg">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 class="gmail_msg"></span></div></div></div></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div></div><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">Sorta.</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">First, graph traits doesn't work on anything but pointers.</div></div></div></div></blockquote><div><br></div><div>Ah, I thought Tim (added Tim Shen to this thread) made this not be the case anymore recently, I forget what the motivation was, and maybe it didn't end up being needed/done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">  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 class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">In any case, this particular issue is:</div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">the child iterators are expected to return noderefs, but it looks like the iterator for clang's CFG returns a value type.<br class="gmail_msg"></div><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">IE the iterators return, say, BasicBlock * (a pointer type).</div><div class="gmail_msg">Clang's returns AdjacentCFGBlock  (a value type).</div></div></div></div></blockquote><div><br></div><div>I'm confused then - sounds like a contradiction, that CFG does have a NodeRef that's not a pointer.<br><br>*goes to look* Ah, I see, the NodeRef is a pointer type, but the child dereference doesn't produce that, instead it produces something that's convertible to a pointer. It's not much bigger - it's two pointers big instead of one pointer.<br><br>And... I see what/why it's doing. Yeah, that. (ramble ramble) Yeah, wouldn't be hard to add an iterator adapter in there to force the conversion so its op*/value_type is /actually/ CFGBlock* instead of something convertible to it.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">Thus, if i use "auto *" instead of "const auto &", it fails.</div></div></div></div></blockquote><div><br>Right - from the comment I was reading it as "using const auto & instead of auto" - wasn't thinking about "const auto *" as the alternative that was being compared/contrasted.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg"><div class="gmail_extra gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">I believe clang is broken here, but honestly, it's hard to tell.<br></div></div></div></div></blockquote><div><br></div><div>Fair.</div><div> </div></div></div>