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

David Blaikie via llvm-commits llvm-commits at lists.llvm.org
Thu Aug 4 11:24:37 PDT 2016

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});
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)


More information about the llvm-commits mailing list