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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added inline comments.


================
Comment at: include/llvm/IR/CFGDiff.h:59
+  }
+
+  bool ignoreChild(const NodePtr BB, NodePtr EdgeEnd, bool Inverse) const {
----------------
timshen wrote:
> 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`.
I added the LegalizeUpdates call that should guarantee the specified behavior. Let me know if this is a good solution.


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


Repository:
  rL LLVM

https://reviews.llvm.org/D50479