[PATCH] D131448: Introduce iterator sentinel to make graph traversal implementation more efficient and cleaner

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Direction seems pretty good - some of the details I'm fuzzy on. If there's easy ways to keep some of the existing syntax (like I see at least one loop that I think was querying the incremented iterator for "isAtEnd" - maybe that can be preserved initially) to reduce the number of places this patch has to touch, then incremental patches after this, and then removing the backcompat API?



================
Comment at: llvm/include/llvm/ADT/BreadthFirstIterator.h:128-131
+  bool operator==(iterator_sentinel) const { return VisitQueue.empty(); }
+
+  bool operator!=(iterator_sentinel RHS) const { return !(*this == RHS); }
+
----------------
Generally any operator that can be a non-member should be a non-member (but can still be a friend) so there's equal conversion handling for LHS and RHS. Could you make these non-members? (maybe a separate patch to do the same to the existing op overloads, so the new ones don't look weird)

do you need the inverse operators too, so the sentinel can appear on either side of the comparison? 


================
Comment at: llvm/include/llvm/ADT/SCCIterator.h:49-50
   using SccTy = std::vector<NodeRef>;
-  using reference = typename scc_iterator::reference;
+  using reference = const SccTy &;
+  using pointer = const SccTy *;
 
----------------
does this need a `value` type too? (& then define the `reference` and `pointer` types relative to that)


================
Comment at: llvm/include/llvm/ADT/SCCIterator.h:152-162
+    bool hasCycle() const {
+      assert(!SCC.empty() && "Dereferencing END SCC iterator!");
+      if (SCC.size() > 1)
+        return true;
+      NodeRef N = SCC.front();
+      for (ChildItTy CI = GT::child_begin(N), CE = GT::child_end(N); CI != CE;
+           ++CI)
----------------
I'm not quite following why this requires the proxy object - even after reading the comment above. It looks like this function is entirely in terms of the `SCC` object that's returned from `operator*` - so maybe this could be a free function, called with `hasCycle(*some_iterator)`?


================
Comment at: llvm/include/llvm/ADT/SCCIterator.h:165-170
+  SCCProxy operator*() const {
     assert(!CurrentSCC.empty() && "Dereferencing END SCC iterator!");
     return CurrentSCC;
   }
 
+  SCCProxy operator->() const { return operator*(); }
----------------
I always forget in which cases you're allowed to return a proxy object from an iterator - I thought some iterator concepts (maybe random access is the level at which this kicks in?) that required something that amounts to "there has to be a real object that outlives the iterator"

Could you refresh my memory on that/on why proxy objects are acceptable for this iterator type? (where/how does this iterator declare what concept it models anyway, since this removed the facade helper?)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131448/new/

https://reviews.llvm.org/D131448