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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

Right, I'm defining Inverse but not IsPostDom (from ChildrenGetter). 
Adding a second bool is an easy option, and defining 2 more traits with:
`std::pair<const GraphDiff<Inverse<BasicBlock *>> *, BasicBlock *>>`
and
`std::pair<const GraphDiff<Inverse<BasicBlock *>> *, Inverse<BasicBlock *>>>`
These would set the second bool for the GraphDiff calls to reflect IsPostDom. NodeRef would remain the same.

The children call site in DomTree would look something like:
`children<GraphDiffBBPair>({GD, BB}))`, with
`using GraphDiffBBPair = std::pair<const GraphDiff<BasicBlock*> *, Inverse<BasicBlock*>;` for predecessors in DomTree (and so on for all combos)

I agree that long term this can and should be reused in DomTree, so I'm trying to make it have the right functionality.
Another possibly related issue, is that I'm not reversing the children, as in DomTree, not sure the reason behind that one.

Another issue with the above traits: is there a way to avoid some of the code duplication, since the only differences are the bool arguments to GraphDiff and succ/pred iterators?



================
Comment at: include/llvm/IR/CFGDiff.h:71
+
+  const SmallVectorImpl<NodePtr> *getAddedChildren(const NodePtr BB,
+                                                   bool Inverse) const {
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D50479