<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Thu, Aug 18, 2016 at 2:26 PM David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</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"><div class="gmail_quote"><div dir="ltr">On Thu, Aug 18, 2016 at 1:35 AM Tim Shen <<a href="mailto:timshen@google.com" target="_blank">timshen@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">timshen created this revision.<br>
timshen added a reviewer: dblaikie.<br>
timshen added a subscriber: llvm-commits.<br>
<br>
Using range-based for loop copies the iterator, so it ends up not<br>
updating *Opt.<br>
<br>
Before the change, the DFS result is correct, because Visited keeps track<br>
of visited nodes. It's just DFS is slower, because for every time<br>
toNext() is called, the for loop starts over at child_begin().<br></blockquote></div></div><div dir="ltr"><div class="gmail_quote"><div><br>This description doesn't quite make sense to me - the loop doesn't appear to start over at the beginning. The change causes the loop to go to the initial end, rather than the new end (by caching the original end, rather than testing against the current/new end on each iteration)<br><br>Why are new nodes appearing in the graph through the iteration?<br><br>(or I'm misunderstanding this in some way(s))<br></div></div></div></blockquote><div><br></div><div>There are several things mixed together:</div><div>1) DepthFirstIteartor does explicitly support graph mutation during the iteration. See r76869.</div><div>2) I didn't notice that with the range-based for loop, end is cached. But not caching the end seems to be the right thing to do.</div><div>3) More importantly, the purpose of this patch is to use "auto &It = *Opt", not "auto It = Opt", as range-based for loop will do. So that Visited.back().second is actually updated before toNext() exits.</div><div><span style="line-height:1.5"> </span><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> </div></div></div><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<a href="https://reviews.llvm.org/D23649" rel="noreferrer" target="_blank">https://reviews.llvm.org/D23649</a><br>
<br>
Files:<br>
  include/llvm/ADT/DepthFirstIterator.h<br>
<br>
Index: include/llvm/ADT/DepthFirstIterator.h<br>
===================================================================<br>
--- include/llvm/ADT/DepthFirstIterator.h<br>
+++ include/llvm/ADT/DepthFirstIterator.h<br>
@@ -107,7 +107,8 @@<br>
       if (!Opt)<br>
         Opt.emplace(GT::child_begin(Node));<br>
<br>
-      for (NodeRef Next : make_range(*Opt, GT::child_end(Node))) {<br>
+      for (auto &It = *Opt; It != GT::child_end(Node); ++It) {<br>
+        NodeRef Next = *It;<br>
         // Has our next sibling been visited?<br>
         if (this->Visited.insert(Next).second) {<br>
           // No, do it now.<br>
<br>
<br>
</blockquote></div></div></blockquote></div></div>