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