[PATCH] D153058: [clang][CFG] Support construction of a weak topological ordering of the CFG.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.

Thanks, this is much simpler!
Just nits & apologies for the delay.



================
Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:24
 #include "llvm/ADT/DenseSet.h"
+#include <deque>
+#include <map>
----------------
some of these added headers are now unused: map, set, variant


================
Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:32
 namespace clang {
-
+namespace internal {
 // An interval is a strongly-connected component of the CFG along with a
----------------
nit: `namespace internal` should probably be moved below the public API


================
Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:104
+/// intervals) if and only if it is reducible (its limit flow graph has one
+/// node). Returns `nullop` when `Cfg` is not reducible.
+///
----------------
nit: nullopt


================
Comment at: clang/include/clang/Analysis/Analyses/IntervalPartition.h:104
+/// intervals) if and only if it is reducible (its limit flow graph has one
+/// node). Returns `nullop` when `Cfg` is not reducible.
+///
----------------
sammccall wrote:
> nit: nullopt
what do we expect to do in practice when the CFG is not reducible? or do we expect that to never happen?

(mostly wondering if we need a fallback)


================
Comment at: clang/lib/Analysis/IntervalPartition.cpp:36
+
+// Requires: `Node::succs()` and `Node::preds()`.
+template <typename Node>
----------------
Might be more useful to say concretely: Nodes are either CFGBlock or CFGIntervalNode


================
Comment at: clang/lib/Analysis/IntervalPartition.cpp:105
+void fillIntervalNode(CFGIntervalGraph &Graph,
+                      std::map<const Node *, CFGIntervalNode *> &Index,
+                      std::queue<const Node *> &Successors,
----------------
std::map of pointers is a bit suspicious, densemap?


================
Comment at: clang/unittests/Analysis/IntervalPartitionTest.cpp:91
+
+MATCHER_P(BlockID, ID, "") { return arg->getBlockID == ID; }
+MATCHER_P(IntervalID, ID, "") { return arg->ID == ID; }
----------------
nit: matcher factories are functions and so should be lowerCamelCase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153058