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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Some drive by comments - up to you & Dean how you prioritize them, etc.



================
Comment at: include/llvm/XRay/Graph.h:79-81
+  /// The functions defined on this type are strictly to enable references given
+  /// by dereferencing an InnerEdgeMapT::iterator to look like a
+  /// std::pair<EdgeIdentifier, EdgeAttribute>, whilst still allowing fast
----------------
I don't understand this explanation - I'm not sure how these member functions would "enable references to look like std::pair". Could you explain further?


================
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() {
----------------
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.


================
Comment at: include/llvm/XRay/Graph.h:105
+  /// VertexIdentifier of the end vertex of the edge.
+  typedef class DenseMap<VertexIdentifier, EdgeAttribute, DMI, EdgeValueType>
+      InnerEdgeMapT;
----------------
Drop the 'class' keyword in all these typedefs


================
Comment at: include/llvm/XRay/Graph.h:165-171
+    template <bool IsConstDest,
+              typename = typename std::enable_if<IsConstDest && !IsConst>::type>
+    operator InEdgeIteratorInt<IsConstDest, BaseIt, const EdgeValueType>()
+        const {
+      return InEdgeIteratorInt<IsConstDest, BaseIt, const EdgeValueType>(
+          this->I, MP, SI);
+    }
----------------
Seems more common, perhaps a bit simpler/more obvious, to implement non-const iterator to const iterator conversion as a converting ctor rather than a conversion operator. Though probably no real concern either way, I guess? Haven't thought about it in great detail before.


================
Comment at: include/llvm/XRay/Graph.h:179
+          MP(_MP), SI(_SI) {}
+    T &operator*() { return *(MP->find(*(this->I))->second.find(SI)); }
+  };
----------------
Should probably be a const member


================
Comment at: include/llvm/XRay/Graph.h:187
+  /// Has a const EdgeValueType as its value_type
+  typedef class InEdgeIteratorInt<true> ConstInEdgeIterator;
+
----------------
Drop the 'class' keyword here as it's not needed, I think? (& similarly below)


================
Comment at: include/llvm/XRay/Graph.h:230
+    InEdgeView(GraphT &_G, VertexIdentifier _A)
+        : M(_G.Edges), A(_A), InvG(_G.InvGraph){};
+  };
----------------
Spurious semicolon

(actually every member function in this class has a semicolon at the end - probably need to do a pass to remove those across this change/others)


================
Comment at: include/llvm/XRay/Graph.h:259-260
+  public:
+    Iterator begin() { return G.Vertices.begin(); }
+    Iterator end() { return G.Vertices.end(); }
+    size_type size() const { return G.Vertices.size(); }
----------------
const overloads?


================
Comment at: include/llvm/XRay/Graph.h:263
+    bool empty() const { return G.Vertices.empty(); }
+    VertexView(GraphT &_G) : G(_G){};
+  };
----------------
Spurious semicolon


================
Comment at: include/llvm/XRay/Graph.h:316-318
+    bool operator==(const const_iterator &b) const {
+      return (TopIt == b.TopIt && (TopIt == TopEnd || BotIt == b.BotIt));
+    };
----------------
where possible, make operator overloads non-members (this ensures equal treatment of the LHS and RHS - as-is, this wouldn't be usable for comparisons with const_iterator on the LHS, for example)


================
Comment at: include/llvm/XRay/Graph.h:330
+
+    reference operator*() { return *BotIt; };
+  };
----------------
this should probably be const?


================
Comment at: include/llvm/XRay/Graph.h:402-405
+    if (count(I) == 0) {
+      VertexValueType VVT = {};
+      VVT.first = I;
+      insert(VVT);
----------------
Does two lookups (count and insert). Avoid that if you can (usually by using "insert" and checking the bool element of the pair returned)


================
Comment at: include/llvm/XRay/Graph.h:414-417
+    if (count(I) == 0) {
+      EdgeValueType EVT = {};
+      EVT.first = I;
+      insert(EVT);
----------------
Similar double lookup


================
Comment at: include/llvm/XRay/Graph.h:496-514
+  Graph &operator=(const Graph &other) {
+    using std::swap;
+    Graph tmp(other);
+    swap(*this, other);
+    return *this;
+  }
+
----------------
Could use the unified assignment operator if you like, maybe (pass by value, then swap)


================
Comment at: include/llvm/XRay/Graph.h:528-529
+  std::pair<EdgeIterator, bool> insert(const EdgeValueType &Val) {
+    operator[](Val.first.first);
+    operator[](Val.first.second);
+    InnerEdgeMapT &TRef = Edges[Val.first.first];
----------------
calling insert might be more obvious. Explicitly calling operator overloads is generally a bit 'weird'.


https://reviews.llvm.org/D29005