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

[Alexis Shaw via Phabricator via llvm-commits]

varno marked 13 inline comments as done.
varno added inline 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() {
----------------
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.


================
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);
+    }
----------------
dblaikie wrote:
> 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.
In this case I cannot define this as a converting ctor, as I need to access a protected member of my parent, which I cannot do in a converting ctor :(=.


================
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));
+    };
----------------
dblaikie wrote:
> 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)
Well actually in this case yes, it is defined symetrically due to the Boolean template behavior of this class. however this does expose a problem in iterator_facade_base which I have to work around.


https://reviews.llvm.org/D29005