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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
