[PATCH] D23522: [ADT] Change PostOrderIterator to use NodeRef. NFC.

[Tim Shen via llvm-commits]

timshen added inline comments.

================
Comment at: include/llvm/ADT/PostOrderIterator.h:93-95
@@ +92,5 @@
+class po_iterator
+    : public std::iterator<std::forward_iterator_tag, typename GT::NodeRef,
+                           ptrdiff_t, typename GT::NodeRef,
+                           typename GT::NodeRef>,
+      public po_iterator_storage<SetType, ExtStorage> {
----------------
dblaikie wrote:
> Having T == Pointer == Reference seems wrong. Could you explain how that works?
This is because po_iterator has "non-standard" operator*() and operator->().

Ideally, value_type should be NodeRef, reference type is NodeRef& and pointer_type is NodeRef*, but that way the current operator*() and operator->()'s returned types don't suffice.

I think it's an orthogonal issue apart from this patch.

================
Comment at: include/llvm/ADT/PostOrderIterator.h:126
@@ -123,3 +125,3 @@
       : po_iterator_storage<SetType, ExtStorage>(S) {
-    if (this->insertEdge((NodeType*)nullptr, BB)) {
+    if (this->insertEdge(Optional<NodeRef>(), BB)) {
       VisitStack.push_back(std::make_pair(BB, GT::child_begin(BB)));
----------------
dblaikie wrote:
> Optional<T>() => None
> 
> (& in a few other places)
The conversion doesn't work here, because po_iterator_storage has insertEdge as a template function on NodeRef.

We can move the "template <typename NodeRef>" from insertEdge up to po_iterator_storage, but that involves other po_iterator_storage template changes as well.


https://reviews.llvm.org/D23522