[PATCH] D23146: [ADT] Migrate DepthFirstIterator to use NodeRef

[David Blaikie via llvm-commits]

dblaikie added a comment.

Removing the ability to compare non-end iterators seems like a loss of functionality & would be pretty tricky for the average user (would catch them by surprise that only end iterators are comparable).

Unless there's a reason the design already precludes comparing non-end iterators, I'd say it's best to preserve the existing behavior there.

Other than that, it looks pretty reasonable.


================
Comment at: include/llvm/ADT/DepthFirstIterator.h:111
@@ +110,3 @@
+
+      if (!Opt.hasValue())
+        Opt.emplace(GT::child_begin(Node));
----------------
I'd just write this as "if (!Opt)" - but up to you.

================
Comment at: include/llvm/ADT/DepthFirstIterator.h:114-117
@@ -112,4 +113,6 @@
+
+      ChildItTy &It = *Opt;
 
       while (It != GT::child_end(Node)) {
-        NodeType *Next = *It++;
+        NodeRef Next = *It++;
         // Has our next sibling been visited?
----------------
For a separate patch: any idea why this isn't a normal for (or even range-for) loop?

================
Comment at: include/llvm/ADT/DepthFirstIterator.h:121
@@ -117,4 +120,3 @@
           // No, do it now.
-          VisitStack.push_back(
-              std::make_pair(PointerIntTy(Next, 0), GT::child_begin(Next)));
+          VisitStack.push_back(StackElement{Next, None});
           return;
----------------
We aren't really using braced init like that much in the project so far. I'd probably stick with () when calling a ctor. (same feedback elsewhere)


https://reviews.llvm.org/D23146