[PATCH] D50479: Expose CFG Update struct. Define GraphTraits to get children given a snapshot CFG.

[Tim Shen via Phabricator via llvm-commits]

timshen added inline comments.


================
Comment at: include/llvm/IR/CFGDiff.h:59
+  }
+
+  bool ignoreChild(const NodePtr BB, NodePtr EdgeEnd, bool Inverse) const {
----------------
Can we have a `void verify() const` that verifies the class invariants? e.g. whether duplicated edges are allowed, can one edge exist in both delete and insert, etc. We can call it with the guards of `#ifdef EXPENSIVE_CHECKS`.


================
Comment at: include/llvm/IR/CFGDiff.h:71
+
+  const SmallVectorImpl<NodePtr> *getAddedChildren(const NodePtr BB,
+                                                   bool Inverse) const {
----------------
asbirlea wrote:
> kuhar wrote:
> > Why is returning a pointer better than reference? Can this ever return nullptr?
> I really wanted to define the GraphTraits in the other header as a pair of a *const* GraphDiff* and BasicBlock*. This meant that I needed to have the methods const as well and they cannot return a reference. No, this can never return null.
> 
> The alternative to all this is to drop the const in the GraphTraits std::pair below and all the related const attributes. Personal preference is to keep them, but if folks have strong preferences otherwise, I'm open to changing it.
Would it be better to return `iterator_range(SmallVectorImpl<NodePtr>::const_iterator)`?


================
Comment at: include/llvm/IR/CFGDiff.h:110
+template <>
+struct GraphTraits<std::pair<const GraphDiff<BasicBlock *> *, BasicBlock *>> {
+  using DataRef = const GraphDiff<BasicBlock *> *;
----------------
Optional: The two GraphTraits specializations are highly duplicated. Maybe find a way to de-duplicate them?

The only differences between the two specializations are (1) succ_iterator vs succ_iterator, (2) succ_begin/succ_end vs pred_begin/pred_end, and (3) false vs true gets passed to getAddedChildren.

Both (1) and (2) can be abstracted in a common place. Maybe add some "inverse utility"?


================
Comment at: include/llvm/Support/CFGUpdate.h:22
+
+template <typename NodePtr> struct Update {
+  using NodeKindPair = PointerIntPair<NodePtr, 1, UpdateKind>;
----------------
Due to the use of `PointerIntPair`, it's better be a class and hides the data members.


================
Comment at: include/llvm/Support/CFGUpdate.h:38
+
+  friend raw_ostream &operator<<(raw_ostream &OS, const Update &U) {
+    OS << (U.getKind() == UpdateKind::Insert ? "Insert " : "Delete ");
----------------
Can this be a `print()` and/or `dump()` function, consistent with other types?


Repository:
  rL LLVM

https://reviews.llvm.org/D50479