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

[Alexis Shaw via Phabricator via llvm-commits]

varno added inline comments.


================
Comment at: include/llvm/XRay/Graph.h:41-44
+///  - VI, this is a type over which DenseMapInfo is defined and is the type
+///    used look up strings, available as VertexIdentifier.
+///  - If the built in DenseMapInfo is not defined, provide a specialization
+///    class type here.
----------------
dberris wrote:
> This seems like it's leaking implementation details. Why should the user of the type need to know about DenseMapInfo?
DenseSet and other Derived classes seem to do this too. 
It does leak some implementation details, but really only that we use DenseMap,
>From that you get the ability to implement for more VertexIdentifier types, the limitation
for key would be the same anyway.


================
Comment at: include/llvm/XRay/Graph.h:98-112
+    VertexIdentifier &getFirst() {
+      return std::pair<EdgeIdentifier, EdgeAttribute>::first.second;
+    }
+
+    const VertexIdentifier &getFirst() const {
+      return std::pair<EdgeIdentifier, EdgeAttribute>::first.second;
+    }
----------------
dberris wrote:
> We still don't have documentation as to "why" these member functions exist, and why we're not just using a `DenseMap<pair<VertexIdentifier, VertexIdentifier>, EdgeAttribute>`. This still isn't clear from the documentation that's been so far mentioned here. Also it's not clear why the adjacency list being stored isn't just an `unordered_map<VertexIdentifier, pair<VertexIdentifier, EdgeAttribute>>`, and lookup for all the adjacent vertices (or even out-edges) will be given by `equal_range(VertexIdentifier)`.
> 
> Since we're already adapting iterators anyway, I'm sure you can turn an iterator that yields a `pair<VertexIdentifier, pair<VertexIdentifier, EdgeAddtribute>>` into something that yields a `tuple<VertexIdentifier&, VertexIdentifier&, EdgeAttribute&>` with appropriate `const`-ness of members.
> 
> For in-edge and out-edge and attribute storage, you could have done it this way:
> 
> ```
> using EdgeAttributeContainer = std::list<EdgeAttribute>;
> std::list<EdgeAttribute> EdgeAttributes;
> unordered_multimap<VertexIdentifier, pair<VertexIdentifier, EdgeAttributeContainer::iterator>> OutEdges;
> unordered_multimap<VertexIdentifier, pair<VertexIdentifier, EdgeAttributeContainer::iterator>> InEdges;
> unordered_multimap<pair<VertexIdentifier, VertexIdentifier>, EdgeAttributeContainer::iterator>> EdgeLookup;
> ```
> 
> Since you're already defining your own iterators, they could have been simpler lookup, and still have the consistent interface.
> 
> I'm not saying this is how you should do it, I'm asking why this isn't what's being done, and the documentation (either internal for the implementer/reader or external generated from this type) isn't answering this question.
I cannot quite do this, however something simmilar probably could be done. (The problem with unordered_multimap is the cost of edge-deletion :(. The code complexity of this would increase slightly. If we were to go this way I would probably implement it more like:

```
DenseMap<VertexIdentifier, Set<VertexIdentifier>> OutEdges;
DenseMap<VertexIdentifier, Set<VertexIdentifier>> InEdges;
DenseMap<pair<VertexIdentifier, VertexIdentifier>, EdgeAttribute>> EdgeLookup;
```

Which would increase the size somewhat, but would probably work fine.

If you want I can go ahead with that?


https://reviews.llvm.org/D29005