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

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: include/llvm/XRay/Graph.h:130
+  /// easy iteration and iterators.
+  struct EdgeValueType : public std::pair<EdgeIdentifier, EdgeAttribute>{
+    VertexIdentifier &getFirst() {
----------------
varno wrote:
> dberris wrote:
> > Why the inheritance? Will this not work with composition instead? Or is this for the empty base optimisation?
> It was not required, fixed.
I still see it doing inheritance -- was this an oversight?


================
Comment at: include/llvm/XRay/Graph.h:145-163
+  ///Typedefs of the various structure types in this class.
+  typedef class DenseMap<VertexIdentifier,
+                             EdgeAttribute,
+                             DenseMapInfo<VertexIdentifier>,
+                             EdgeValueType> InnerEdgeMapT;
+  typedef class DenseMap<VertexIdentifier, InnerEdgeMapT> EdgeMapT;
+  typedef class DenseMap<VertexIdentifier, VertexAttribute> VertexMapT;
----------------
dberris wrote:
> I may be wrong, but there seems to be a slight preference for `using` instead of `typedef` now.
i.e. instead of:

`typedef Foo X;`

say:

`using X = Foo;`


================
Comment at: include/llvm/XRay/Graph.h:432-435
+    for(auto &V : b.vertices())
+      insert(V);
+    for(auto &E : b.edges())
+      insert(E);
----------------
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.


================
Comment at: include/llvm/XRay/Graph.h:10
+//
+// A Graph Datatype for the llvm-xray graph sub command
+//
----------------
This isn't entirely accurate -- now that this is in include/llvm/XRay/... then it's no longer just for the `llvm-xray graph` subcommand.


================
Comment at: include/llvm/XRay/Graph.h:30
+
+/// A Graph object represents a Directed Graph and is used in XRAY to compute
+/// and store function call graphs and associated statistical information.
----------------
s/XRAY/XRay/


================
Comment at: include/llvm/XRay/Graph.h:34
+/// The graph takes in four template parameters, these are:
+///  - VertexAttribute, this is a structure which is stored for each variable.
+///    Must be DefaultConstructible, CopyConstructible, CopyAssignable and
----------------
Each variable? Or each Vertex?


================
Comment at: include/llvm/XRay/Graph.h:47
+/// Usage Example Graph with weighted edges and vertices:
+///   auto G = Graph<int, int, int>();
+///
----------------
Why this, as opposed to:

`Graph<int, int, int> G;`


================
Comment at: include/llvm/XRay/Graph.h:59
+///   }
+///   auto G_2(G); //Copy G;
+///
----------------
You don't really need this copy example, just state that `Graph` is CopyConstructible, CopyAssignable, MoveConstructible, and MoveAssignable (but has no strict weak partial ordering nor equality, which may or may not be something you'd want to implement anyway).


================
Comment at: include/llvm/XRay/Graph.h:75-86
+  /// EdgeValueType, a child type of std::pair<EdgeIdentifier, EdgeAttribute>,
+  /// and should be treated as such. This type the value_type of all iterators
+  /// which range over sets of edges.
+  ///
+  /// 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
----------------
s/whilst/while/

Break up the run-on sentence into shorter sentences too.


================
Comment at: include/llvm/XRay/Graph.h:85
+  /// neighborhood of a vertex, whilst maintaining a consistent and easy to
+  /// understand API. Please don't use them.
+  /// FIXME: figure out the friend declaration to enable them to be private.
----------------
s/Please don't use them.//


================
Comment at: include/llvm/XRay/Graph.h:278
+                                      EdgeValueType>::type> {
+  private:
+    typedef
----------------
For classes, it's already implicit that members are private unless otherwise specified. You may omit this, and the one following it in line 293.


================
Comment at: include/llvm/XRay/Graph.h:300
+    EdgeListIterator(TopItT _TopIt, TopItT _TopEnd, BotItT _BotIt)
+        : TopIt(_TopIt), TopEnd(_TopEnd), BotIt(_BotIt){};
+
----------------
Space before the `{`.


================
Comment at: include/llvm/XRay/Graph.h:304
+        : TopIt(_TopIt), TopEnd(_TopEnd) {
+      BotIt = (TopIt == TopEnd) ? BotItT() : TopIt->second.begin();
+    }
----------------
This initialisation can happen in the initializer list too.


================
Comment at: include/llvm/XRay/Graph.h:483
+    InnerEdgeMapT &TRef = Edges[Val.first.first];
+    typename EdgeMapT::iterator TI = Edges.find(Val.first.first);
+    InnerInvGraphT &ITRef = InvGraph[Val.first.second];
----------------
Maybe use 'auto' here instead?

```
auto TI = Edges.find(Val.first.first);
```

Also, alternatively you can alias the members of the pairs with more descriptive names.


================
Comment at: tools/llvm-xray/xray-graph.h:148
                         StatType VertexColor = StatType::NONE);
+  GraphT getGraph() {
+    calculateEdgeStatistics();
----------------
Empty line before?


https://reviews.llvm.org/D29005