[PATCH] D23625: [Analysis] Change several Analysis pieces to use NodeRef. NFC.

[Tim Shen via llvm-commits]

timshen added inline comments.

================
Comment at: include/llvm/Analysis/RegionIterator.h:35
@@ -34,3 @@
-class RNSuccIterator : public std::iterator<std::forward_iterator_tag,
-                                           NodeType, ptrdiff_t> {
-  typedef std::iterator<std::forward_iterator_tag, NodeType, ptrdiff_t> super;
----------------
dblaikie wrote:
> If it has to be a pointer type anyway - would it be simpler to preserve that behavior of value_type being a pointer rather than a value? Or is there something else going on here as well that makes this change necessary/useful?
Actually by looking at the code, I can tell that NodeType is always GraphTraits<BlockT*>::NodeType. After the migration I prefer to keep the consistency, that is the template parameter NodeRef is always GraphTraits<BlockT*>::NodeRef. The fact that NodeRef is required to be a pointer seems not worthy to be mentioned too frequently.

The code also assumes that BlockT* is NodeRef/NodeType*. And the code also uses getParent(). I'm sad that people stick on GraphTraits just because it exists, when TreeTraits could be a better fit.


https://reviews.llvm.org/D23625