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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

To reduce the number of allocations, have you thought about making EdgeList contain the edges objects instead of pointers? The edges would have to be copied/moved into the list and edges could not be compared by identity. Is this semantic needed/are edge objects large?

Since the edge class does not contain the source node, the same edge object could be put into multiple outgoing edges lists. Is this supported?



================
Comment at: llvm/include/llvm/ADT/DirectedGraph.h:9
+//
+// This file defines the interface for a directed graph.
+//
----------------
Not just the interface; also a base implementation.


================
Comment at: llvm/include/llvm/ADT/DirectedGraph.h:54
+
+  NodeType &Node; // the target node this edge connects to
+};
----------------
Could this get a more descriptive name, such as `Target`/`TargetNode`?


================
Comment at: llvm/include/llvm/ADT/DirectedGraph.h:61
+public:
+  using EdgeList = SmallVector<EdgeType *, 4>;
+  using iterator = typename EdgeList::iterator;
----------------
[style] Type declarations typically have the suffix `Ty`


================
Comment at: llvm/include/llvm/ADT/DirectedGraph.h:65
+
+  // Create a node with a single outgoing edge \p E.
+  explicit DGNode(EdgeType &E) : Edges() { Edges.push_back(&E); }
----------------
Make a doxygen comment?


================
Comment at: llvm/include/llvm/ADT/DirectedGraph.h:72
+
+  DGNode<NodeType, EdgeType> &operator=(const DGNode<NodeType, EdgeType> &N) {
+    Edges = N.Edges;
----------------
There is a move constructor, but no move-assignment overload.


================
Comment at: llvm/include/llvm/ADT/DirectedGraph.h:80
+  bool operator==(const NodeType &N) const {
+    return static_cast<const NodeType *>(this)->isEqualTo(N);
+  }
----------------
[style] I often create a protected member `NodeType &getDerived()` (an `const NodeType &getDerived() const`) to avoid casting on every use. See e.g. `clang::ASTNodeTraverser::getDerived()` for the technique. 


================
Comment at: llvm/include/llvm/ADT/DirectedGraph.h:93
+  EdgeType &front() {
+    return const_cast<EdgeType &>(static_cast<const DGNode &>(*this).front());
+  }
----------------
I understand you might want to re-use one implementation, but `return *Edges.front()` is a lot shorter (The assertion is redundant: SmallVector::front() already contains it).


================
Comment at: llvm/include/llvm/ADT/DirectedGraph.h:118-120
+    if (llvm::find_if(Edges, [&E](const EdgeType *Edge) {
+          return *Edge == E;
+        }) != Edges.end())
----------------
This makes me worry about scaling. Is this method used often? How large is the worst practical edge list? Did you think about using `SetVector`?

For the sake of avoiding premature optimization, we might worry about it when it becomes a problem.


================
Comment at: llvm/include/llvm/ADT/DirectedGraph.h:129-134
+    auto IT =
+        llvm::find_if(Edges, [&E](const EdgeType *Edge) { return *Edge == E; });
+    if (IT == Edges.end())
+      return false;
+    Edges.erase(IT);
+    return true;
----------------
Use `std::remove`?


================
Comment at: llvm/include/llvm/ADT/DirectedGraph.h:209
+
+  unsigned size() const { return Nodes.size(); }
+
----------------
Return `size_t`?


================
Comment at: llvm/include/llvm/ADT/DirectedGraph.h:275-279
+    Src.findEdgesTo(Dst, EL);
+    if (llvm::find_if(EL, [&E](const EdgeType *Edge) { return *Edge == E; }) !=
+        EL.end())
+      return false;
+    Src.addEdge(E);
----------------
This iterates over the list of outgoing edges 3 times. The `find_if` is redundant since `addEdge` already does this.


================
Comment at: llvm/include/llvm/ADT/DirectedGraph.h:287
+    for (NodeType *N : Nodes)
+      N->clear();
+    Nodes.clear();
----------------
Is clearing each individual node necessary? I wouldn't expect them to be re-used.


================
Comment at: llvm/include/llvm/ADT/DirectedGraph.h:292
+protected:
+  NodeList Nodes; // the nodes in the graph
+};
----------------
Comment before the member.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64088