[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:432-435
+    for(auto &V : b.vertices())
+      insert(V);
+    for(auto &E : b.edges())
+      insert(E);
----------------
dberris wrote:
> varno wrote:
> > 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.)
> Please say more -- it's not clear why an explicit copy of the members doesn't just work.
Oh, in the new version it does work. That is why there is no constructors non-default anymore :)


================
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:
> varno wrote:
> > 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.
> I'm a little uneasy with the requirements being imposed on VI here. Let me ask a different question: can we just remove this? Why is this essential if we're not relying on DenseMap requirements anymore in the implementation? And if we are using DenseMap, can we get away with just not even mentioning this type in the interface of Graph?
> 
> I'm trying to further simplify this type because it's currently really hard to explain why VI is there just by looking at what this type is supposed to do.


We are still using DenseMap, and so the question.

VI needs to be there, because the type is int for Graph and StrRef for Graph Diff.


================
Comment at: include/llvm/XRay/Graph.h:315
+  void
+  clear() { // will leak if VI is not A POD type becasue of DenseMap problems.
+    Edges.clear();
----------------
dberris wrote:
> I'd remove this comment.
It was not a problem anymore anyway.


https://reviews.llvm.org/D29005