[PATCH] D29320: [XRay] A tool for Comparing xray function call graphs

[Dean Michael Berris via Phabricator via llvm-commits]

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

Please address all comments by me and @dblaikie



================
Comment at: tools/llvm-xray/xray-graph-diff.cc:222
+
+  for (int i = 0; i < 2; ++i)
+    for (const auto &V : G[i].get().vertices())
----------------
Magic number 2 for a reason? Maybe use the N in GraphDiffRenderer?


================
Comment at: tools/llvm-xray/xray-graph-diff.cc:261
+// which ranges onto [-infty, infty] with the sign corresponding to which number
+// is larger
+static double difference(double A, double B) { return A / B - B / A; }
----------------
Punctuation mark?


================
Comment at: tools/llvm-xray/xray-graph-diff.cc:316
+                            GraphDiffRenderer::StatType T) {
+  if (E.second.P[0] == nullptr)
+    return "green";
----------------
What would be really helpful here is if you gave `E.second` a name. Is this an `EdgeAttr`? If so, if would be much easier to read something like:

```
auto &EdgeAttr = E.second;
if (EdgeAttr.Parents[0] == nullptr)
  return "green";
if (EdgeAttr.Parents[1] == nullptr)
  return "red";
...
  EdgeAttr.Parents[0].Stats.getDouble(Stat)
```


================
Comment at: tools/llvm-xray/xray-graph-diff.cc:327-329
+  if (MR == 0.0 && R == 0.0)
+    MR = 1.0;
+
----------------
I'm not an expert here, but comparing to zero seems a little... odd because these are floating point numbers, but maybe not something important enough to worry about.


================
Comment at: tools/llvm-xray/xray-graph-diff.cc:416-418
+  if (A.getPosition() == 0 && AA.getPosition() == 0) {
+    return B;
+  }
----------------
Lose the `{` and `}` ?


================
Comment at: tools/llvm-xray/xray-graph-diff.h:51
+    std::array<std::reference_wrapper<const GraphRenderer::GraphT>,
+               GraphDiffRenderer::N>
+        G;
----------------
Do you need to fully qualify N here?


https://reviews.llvm.org/D29320