[PATCH] D29005: [XRay] A graph Class for the llvm-xray graph

[Alexis Shaw via Phabricator via llvm-commits]

varno marked 9 inline comments as done.
varno added inline comments.


================
Comment at: include/llvm/XRay/Graph.h:130
+  /// easy iteration and iterators.
+  struct EdgeValueType : public std::pair<EdgeIdentifier, EdgeAttribute>{
+    VertexIdentifier &getFirst() {
----------------
dberris wrote:
> Why the inheritance? Will this not work with composition instead? Or is this for the empty base optimisation?
It was not required, fixed.


================
Comment at: include/llvm/XRay/Graph.h:131-142
+    VertexIdentifier &getFirst() {
+      return std::pair<EdgeIdentifier, EdgeAttribute>::first.second;
+    };
+    const VertexIdentifier &getFirst() const{
+      return std::pair<EdgeIdentifier, EdgeAttribute>::first.second;
+    };
+    EdgeAttribute &getSecond(){
----------------
dberris wrote:
> Since you're giving names anyway, consider using more descriptive ones (like `getVertexId` and `getEdgeAttr`).
Explained in comment


================
Comment at: include/llvm/XRay/Graph.h:432-435
+    for(auto &V : b.vertices())
+      insert(V);
+    for(auto &E : b.edges())
+      insert(E);
----------------
dberris wrote:
> Is this necessary to implement this way? Can we get away with making copies of the underlying data instead?
> 
> I mean, wouldn't it be valid if we just kept this `= default` also?
Sadly no, it does not work. :( (that was the first thing I tried, and it caused failed tests.)


================
Comment at: include/llvm/XRay/Graph.h:480
+    p.first->first.first = Val.first.first;
+    ++NumEdges;
+    return {EdgeIterator(TI, Edges.end(), p.first), p.second};
----------------
dberris wrote:
> I'm wondering whether you need to keep track of this value yourself (or whether it's something one of the data structures already keeps track of, that you can just return)?
This is not tracked in a single place anywhere, It could be recalculated every time you want to query If you think that would be better. 


https://reviews.llvm.org/D29005