[PATCH] D64088: [DDG] DirectedGraph as a base class for various dependence graphs such as DDG and PDG.

[Bardia via Phabricator via llvm-commits]

bmahjour marked an inline comment as done.
bmahjour added inline comments.


================
Comment at: llvm/include/llvm/ADT/DirectedGraph.h:287
+    for (NodeType *N : Nodes)
+      N->clear();
+    Nodes.clear();
----------------
Meinersbur wrote:
> bmahjour wrote:
> > Meinersbur wrote:
> > > bmahjour wrote:
> > > > Meinersbur wrote:
> > > > > Is clearing each individual node necessary? I wouldn't expect them to be re-used.
> > > > Not sure if I understand your question...we need to clear each node so that their outgoing edges are removed.
> > > But there are no nodes in the graph after `clear()`; it is irrelevant what the previous nodes store, they are not part of the graph anymore.
> > >But there are no nodes in the graph after clear(); it is irrelevant what the previous nodes store, they are not part of the graph anymore.
> > Once the nodes are removed all the handles to the edges become inaccessible, so we need to clear the edges in each node (while we have a chance) before we remove the nodes from the graph.
> > Once the nodes are removed all the handles to the edges become inaccessible, so we need to clear the edges in each node (while we have a chance) before we remove the nodes from the graph.
> 
> Why does the edge list need to be empty for nodes that are not in any graph? It's not for free'ing the SmallVector's memory, that will happen anyway when free'ing the node.
>Why does the edge list need to be empty for nodes that are not in any graph? It's not for free'ing the SmallVector's memory, that will happen anyway when free'ing the node.

Conceptually the graph is a collection of nodes and edges. One would expect the `clear` operation on the graph to behave as if `removeNode` is called on all the nodes which would result in the nodes and connected edges to be removed. I'd find it surprising if `clear` removes the nodes and leaves the edges in place. In any case, I've removed the `clear` function all together, since it's not all that useful anyway.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64088/new/

https://reviews.llvm.org/D64088