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

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: tools/llvm-xray/xray-graph-diff.cc:268
+
+// This is a functino which compares a and b relitively but ranges from
+// -1 to 1 to show the direction of the chenge, it also has the property that
----------------
s/funtino/function/
s/relitively/relatively/ ?


================
Comment at: tools/llvm-xray/xray-graph-diff.cc:271
+// DivF(B,A) = - DivF(A,B);
+static double divF(double A, double B) { return A / B - B / A; }
+
----------------
Now that you've documented this, it seems like this is a "bounded relative diff" or a means of normalizing distance within the range [-1, 1]. I'd urge you to find a more expressive/intuitive name than "divF".


================
Comment at: tools/llvm-xray/xray-graph-diff.cc:371-373
+static Twine trunS(const StringRef &S, size_t n) {
+  return (S.size() > n) ? Twine(S.substr(0, n)) + "..." : Twine(S);
+}
----------------
Am not a fan of this function's naming... consider spelling it out instead, `truncateString` seems just fine to me.


================
Comment at: tools/llvm-xray/xray-graph-diff.h:25
+
+class GraphDiffRenderer {
+  static const int N = 2;
----------------
Please document this class, how to use it, etc.


================
Comment at: tools/llvm-xray/xray-graph-diff.h:26
+class GraphDiffRenderer {
+  static const int N = 2;
+
----------------
nit: really ought to be `constexpr size_t`.


================
Comment at: tools/llvm-xray/xray-graph-diff.h:33
+  struct EdgeAttribute {
+    std::array<const GraphRenderer::GraphT::EdgeValueType *, N> P = {};
+  };
----------------
A `using` statement to bring in this `EdgeValueType` into scope would really make this more readable.


https://reviews.llvm.org/D29320