<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Mon, Aug 22, 2016 at 2:42 PM Tim Shen <<a href="mailto:timshen@google.com">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 added inline comments.<br>
<br>
================<br>
Comment at: unittests/ADT/DepthFirstIteratorTest.cpp:22<br>
@@ +21,3 @@<br>
+public:<br>
+  using iterator = typename SmallPtrSet<T, 4>::iterator;<br>
+<br>
----------------<br>
dblaikie wrote:<br>
> Typedef?<br>
When do we use using and when typedef? If I'm creating a new file I'd go for using.<br></blockquote><div><br></div><div>Pretty sure we use typedef wherever we can - if the style guide says differently, happy to be corrected. (or happy to have a style guide discussion about changing the direction here)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
================<br>
Comment at: unittests/ADT/DepthFirstIteratorTest.cpp:51-53<br>
@@ +50,5 @@<br>
+  StorageT S;<br>
+  for (auto N : make_range(DFIter::begin(G, S), DFIter::end(G, S))) {<br>
+    (void)N;<br>
+  }<br>
+  EXPECT_EQ(3, S.InsertVisited);<br>
----------------<br>
dblaikie wrote:<br>
> Probably drop braces on a single line loop.<br>
><br>
> Though maybe this is better expressed with a different construct?<br>
><br>
>   (void)std::distance(begin, end), perhaps?<br>
><br>
> (saves the make_range, extra variable copies, etc?) Probably with a comment saying something like "walk the DF ordering & ensure each element was only visited once"<br>
std::distance doesn't do what we want on randomaccess iterators,</blockquote><div><br></div><div>Fair point</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> so using std::distance makes it depends more on the context. I'd rather make it more explicit using range-based for loop.<br></blockquote><div><br></div><div>Still a lot under the covers with a range-for. But whatever works for you.<br><br>Commit when ready,<br><br>- Dave</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<a href="https://reviews.llvm.org/D23649" rel="noreferrer" target="_blank">https://reviews.llvm.org/D23649</a><br>
<br>
<br>
<br>
</blockquote></div></div>