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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Few more drive-by comments.



================
Comment at: include/llvm/XRay/Graph.h:87
+  /// FIXME: figure out the friend declaration to enable them to be private.
+  struct EdgeValueType : public std::pair<EdgeIdentifier, EdgeAttribute> {
+    VertexIdentifier &getFirst() {
----------------
varno wrote:
> dblaikie wrote:
> > This seems like a strange class - it doesn't look like it provides any encapsulation (public inheritance) & the names don't provide any extra documentation. I'd suggest using std::pair directly without this extra type.
> Please see the new Comments to explain this class class.
Sorry, it's still unclear to me what this class provides over using std::pair directly. (but it sounds like Dean's in on the discussion and has some higher-level feedback/ideas/etc that might motivate a different direction or help sure up this one)


================
Comment at: include/llvm/XRay/Graph.h:538-539
+  size_type count(const EdgeIdentifier &I) const {
+    return (Edges.count(I.first) == 1)
+               ? Edges.find(I.first)->second.count(I.second)
+               : 0;
----------------
two lookups (count + find)  - avoid that by using find and then checking end, etc.


================
Comment at: include/llvm/XRay/Graph.h:563
+    return Vertices.insert(Val);
+  };
+
----------------
Extra semicolon (there are a whole bunch in this class/generally - remove them all)


================
Comment at: include/llvm/XRay/Graph.h:571
+    auto p2 = p1.second.insert({Val.first.second, Val.second});
+    if(!p2.second){
+      Vertices.insert({Val.first.first, VertexAttribute()});
----------------
Missing spaces (run this all through clang-format?)


================
Comment at: include/llvm/XRay/Graph.h:578
+      ++NumEdges;
+    };
+
----------------
Extra semicolon


================
Comment at: include/llvm/XRay/Graph.h:581
+    return {EdgeIterator(p1.first, Edges.end(), p2.first), p2.second};
+  };
+};
----------------
Extra semicolon


================
Comment at: tools/llvm-xray/xray-graph.cc:497-499
+    if (V.first == 0) {
+      continue;
+    }
----------------
LLVM code usually skips {} on single line statements.


https://reviews.llvm.org/D29005