Add a utility for iterating over all topological orders #6801
Conversation
Use an extension of Kahn's algorithm for finding topological orders that iteratively makes every possible choice at every step to find all the topological orders. The order being constructed and the set of possible choices are managed in-place in the same buffer, so the algorithm takes linear time and space plus amortized constant time per generated order. This will be used in an upcoming type optimization.
| Iterator begin() { return {this}; } | ||
| Iterator end() { return {nullptr}; } | ||
|
|
||
| private: |
There was a problem hiding this comment.
Can all the private parts be moved out of the header? I think it is easier to read in either a single file (no separate cpp; makes sense for short things), or two files with the implementation in one and the interface in the other.
E.g. I think LocalGraph does a good job of this. The header has just the API, and all the flow analysis to compute the graph is in the cpp.
There was a problem hiding this comment.
Not without introducing a layer of indirection, unfortunately. We could add a unique_ptr to an opaque implementation object, but I'd rather not. Do you think it would be worth it?
There was a problem hiding this comment.
Fair enough as it is, I guess. It's reasonably short, and the iteration makes it hard to separate.
| // Find the in-degree of each vertex. | ||
| for (const auto& vertex : graph) { | ||
| for (auto child : vertex) { | ||
| ++indegrees[child]; |
There was a problem hiding this comment.
Once again I wonder why ++x; instead of x++;, when there is nothing reading the old value... 😄
There was a problem hiding this comment.
For iterators with non-trivial state, preincrement is more efficient, so should clearly be preferred. (Optimizers can't necessarily optimize out copies even if there are no readers because the copy constructors might have arbitrary side effects.) For other iterators it doesn't matter, so we should still choose preincrement for the sake of consistency.
| Iterator begin() { return {this}; } | ||
| Iterator end() { return {nullptr}; } | ||
|
|
||
| private: |
There was a problem hiding this comment.
Fair enough as it is, I guess. It's reasonably short, and the iteration makes it hard to separate.
| // position and return the new selector for the next position. Returns | ||
| // nullopt if advancing wraps back around to the original configuration. | ||
| std::optional<Selector> advance(TopologicalOrders& ctx); | ||
| }; |
There was a problem hiding this comment.
I read Kahn's Algorithm on wikipedia but didn't see mention of "selectors" there. Is this a generally familiar term I am unaware of?
There was a problem hiding this comment.
Kahn's algorithm finds a single topological sort by selecting one of the next available vertices arbitrarily. Here we systematically select each of the next available vertices in sequence to enumerate all the possible topological sorts. Selector just encapsulates the logic for making one of those iterative selections. It doesn't come from any established literature.
Use an extension of Kahn's algorithm for finding topological orders that
iteratively makes every possible choice at every step to find all the
topological orders. The order being constructed and the set of possible
choices are managed in-place in the same buffer, so the algorithm takes
linear time and space plus amortized constant time per generated order.
This will be used in an upcoming type optimization.