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

[David Blaikie via llvm-commits]

dblaikie added inline comments.

================
Comment at: unittests/ADT/DepthFirstIteratorTest.cpp:18-21
@@ +17,6 @@
+
+template <typename T> class CountedSet {
+  SmallPtrSet<T, 4> S;
+
+public:
+  using iterator = typename SmallPtrSet<T, 4>::iterator;
----------------
Might just make this a struct? (InsertVisited isn't protected in any way, for example, doesn't seem too important)

================
Comment at: unittests/ADT/DepthFirstIteratorTest.cpp:22
@@ +21,3 @@
+public:
+  using iterator = typename SmallPtrSet<T, 4>::iterator;
+
----------------
Typedef?

================
Comment at: unittests/ADT/DepthFirstIteratorTest.cpp:26
@@ +25,3 @@
+
+  CountedSet() {}
+
----------------
This should be implicit?

================
Comment at: unittests/ADT/DepthFirstIteratorTest.cpp:39
@@ +38,3 @@
+  df_iterator_storage(CountedSet<T> &VSet) : Visited(VSet) {}
+  df_iterator_storage(const df_iterator_storage &S) : Visited(S.Visited) {}
+  CountedSet<T> &Visited;
----------------
Do you need to write this copy ctor? Should be implicit, no?

================
Comment at: unittests/ADT/DepthFirstIteratorTest.cpp:44-45
@@ +43,4 @@
+TEST(DepthFirstIteratorTest, ActuallyUpdateIterator) {
+  using StorageT = CountedSet<Graph<3>::NodeType *>;
+  using DFIter = df_iterator<Graph<3>, StorageT, true>;
+
----------------
I think we just use 'typedef' for this so far.

================
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);
----------------
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"


https://reviews.llvm.org/D23649