[PATCH] D23649: [ADT] Actually mutate the iterator VisitStack.back().second, not its copy.

[Tim Shen via llvm-commits]

timshen added inline comments.

================
Comment at: unittests/ADT/DepthFirstIteratorTest.cpp:22
@@ +21,3 @@
+public:
+  using iterator = typename SmallPtrSet<T, 4>::iterator;
+
----------------
dblaikie wrote:
> Typedef?
When do we use using and when typedef? If I'm creating a new file I'd go for using.

================
Comment at: unittests/ADT/DepthFirstIteratorTest.cpp:51-53
@@ +50,5 @@
+  StorageT S;
+  for (auto N : make_range(DFIter::begin(G, S), DFIter::end(G, S))) {
+    (void)N;
+  }
+  EXPECT_EQ(3, S.InsertVisited);
----------------
dblaikie wrote:
> Probably drop braces on a single line loop.
> 
> Though maybe this is better expressed with a different construct?
> 
>   (void)std::distance(begin, end), perhaps?
> 
> (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"
std::distance doesn't do what we want on randomaccess iterators, so using std::distance makes it depends more on the context. I'd rather make it more explicit using range-based for loop.


https://reviews.llvm.org/D23649