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

Wed Feb 8 23:55:39 PST 2017

dberris requested changes to this revision.
dberris added a comment.
This revision now requires changes to proceed.

Please address all 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.
----------------
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.

================
Comment at: include/llvm/XRay/Graph.h:49-61
+/// Usage Example Graph with weighted edges and vertices:
+///   Graph<int, int, int> G;
+///
+///   G[1] = 0;
+///   G[2] = 2;
+///   G[{1,2}] = 1;
+///   G[{2,1}] = -1;
----------------
dberris wrote:
> Some well-placed empty lines here would help readability.
> 
> ```
> /// Usage:
> ///
> ///    Graph<int, int, int> G;
> ///    G[1] = 0;
> ///    G[2] = 1;
> ///    G[{1, 2}] = 2;
> ///
> ///    for (const auto& V : G.vertices()) {
> ///       // V will be an int, do something with it here.
> ///    }
> ///
> ///    for (const auto &E : G.edges()) {
> ///        // E will be a std::pair<int, int>, do something with it here.
> ///    }
> ///
> ///    // show an example on what to do with the third int?
> ```
I don't see it done.

================
Comment at: include/llvm/XRay/Graph.h:212-213
+      return const_iterator(It->second.begin(), &M, A);
+    }
+    const_iterator begin() const { return cbegin(); }
+
----------------
Empty line in between?

================
Comment at: include/llvm/XRay/Graph.h:220-221
+      return iterator(It->second.end(), &M, A);
+    }
+    const_iterator cend() const {
+      auto It = NL.find(A);
----------------
Empty line in between?

================
Comment at: include/llvm/XRay/Graph.h:230
+    size_type size() const {
+      return (NL.count(A) == 1) ? NL.find(A)->second.size() : 0;
+    }
----------------
You're doing two lookups here. Consider just doing one lookup using iterators.

================
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();
----------------
I'd remove this comment.

================
Comment at: include/llvm/XRay/Graph.h:364
+  EdgeAttribute &operator[](const EdgeIdentifier &I) {
+    auto &p = Edges.FindAndConstruct(I);
+    Vertices.FindAndConstruct(I.first);
----------------
LLVM Style uses capital letters for local variable names.

================
Comment at: tools/llvm-xray/xray-graph.h:125
+      : FuncIdHelper(FuncIdHelper), DeduceSiblingCalls(DSC) {
+    G[0] = {};
+  }
----------------
Probably not for now, but it would be great if we can construct a graph with a terse interface. Something like:

```
Graph<int, int, int> G(
  {1, 2, 3, 4, 5},
  {{1, 2}, {2, 3}, {3, 4}});
```

Maybe write as a TODO on the Graph type?

================
Comment at: unittests/XRay/GraphTest.cpp:1
+//===- llvm/unittest/XRay/GraphTest.cpp - XRay Graph unit tests -*- C++ -*-===//
+//
----------------
Please have more basic tests (construction, copy-construction, assignment, move construction, etc.), smaller and more focused test cases for more interface and semantics-related tests.

================
Comment at: unittests/XRay/GraphTest.cpp:19
+
+// Test hashing with a set of only two entries.
+TEST(GraphTest, DoubleVertexEntryGraphTest) {
----------------
This comment seems nothing to do with the graph. Sets? Hashing? This seems to be leaking implementation details, so let's avoid that by only keeping this about the graph.

================
Comment at: unittests/XRay/GraphTest.cpp:24
+  G.insert({1u,1u});
+  // Original failure was an infinite loop in this call:
+  EXPECT_EQ(0u, G.count(2));
----------------
This comment seems out of place.

================
Comment at: unittests/XRay/GraphTest.cpp:46-47
+  const GraphT  &G = MG;
+  EXPECT_EQ(0u, G.count(0u));
+  EXPECT_EQ(0u, G.count({0u,1u}));
+  auto VE = G.at(0);
----------------
Probably needs to be `ASSERT_EQ` because if this isn't true it doesn't make sense to proceed with the test case.

================
Comment at: unittests/XRay/GraphTest.cpp:50-51
+  auto EE = G.at({0,0});
+  EXPECT_FALSE(VE ); // G.at[0] returns an error
+  EXPECT_FALSE(EE ); // G.at[{0,0}] returns an error
+  handleAllErrors(VE.takeError(), [](const StringError &S){});
----------------
Spurious whitespace.

================
Comment at: unittests/XRay/GraphTest.cpp:52-53
+  EXPECT_FALSE(EE ); // G.at[{0,0}] returns an error
+  handleAllErrors(VE.takeError(), [](const StringError &S){});
+  handleAllErrors(EE.takeError(), [](const StringError &S){});
+  EXPECT_TRUE(G.vertices().empty());
----------------
Use `consumeError(...)` instead?

================
Comment at: unittests/XRay/GraphTest.cpp:60-83
+  {
+  MG[0u] = {1u};
+  EXPECT_EQ(1u, G.count(0u));
+  EXPECT_EQ(0u, G.count(1u));
+  EXPECT_EQ(1u, MG[0u].VA);
+  auto T = G.at(0u);
+  EXPECT_TRUE(!! T  );
----------------
Break out into a different test case, please, give it a name that describes what the case is actually testing.

================
Comment at: unittests/XRay/GraphTest.cpp:66
+  auto T = G.at(0u);
+  EXPECT_TRUE(!! T  );
+  EXPECT_EQ(1u, T->VA);
----------------
Consider `EXPECT_TRUE(bool{T})` instead.

https://reviews.llvm.org/D29005