[PATCH] D28225: Implemented color coding and Vertex labels in XRay Graph

[Dean Michael Berris via Phabricator via llvm-commits]

dberris requested changes to this revision.
dberris added inline comments.
This revision now requires changes to proceed.


================
Comment at: tools/llvm-xray/xray-graph.cc:396
+  return B;
+}
+// Takes a double precision number, clips it between 0 and 1 and then converts
----------------
Empty line after this?


================
Comment at: tools/llvm-xray/xray-graph.cc:398
+// Takes a double precision number, clips it between 0 and 1 and then converts
+// that to an integer between 0x00 and 0xFF with propper rounding.
+static uint8_t uintIntervalTo8bitChar(double B) {
----------------
s/propper/proper/


================
Comment at: tools/llvm-xray/xray-graph.cc:409-410
+//
+// In order to calculate these polynomials, I used Mathematica to do the
+// following:
+//   1. Convert the OrRed9 color scheme from http://colorbrewer2.org/ from sRGB
----------------
Leave out the "I" and "Mathematica" parts -- explain the concepts involved, not the mechanics.


================
Comment at: tools/llvm-xray/xray-graph.cc:416-417
+//   3. Sample this interpolation at 100 points and convert to sRGB.
+//   4. Calculate a polynomial fit for these 100 points for each of R G and B.
+//      using a degree 9 polynominal subjectively provided a good result.
+//   5. Extract these polynomial coefficients from matlab as a set of constants.
----------------
Instead of "subjectively provided a good result", say something like: "we used a 9-degree polynomial arbitrarily based on a fuzzy goodness of fit metric (using human judgment)".


================
Comment at: tools/llvm-xray/xray-graph.cc:430-431
+                                    0.732108, 0.913916};
+  using std::begin;
+  using std::end;
+  uint8_t r = uintIntervalTo8bitChar(polyEval(RedPoly, point));
----------------
Unnecessary now?


================
Comment at: tools/llvm-xray/xray-graph.cc:439-440
+
+// Returns the quotient between the property T of this and annother TimeStat as
+// a double
+double GraphRenderer::TimeStat::compare(StatType T, const TimeStat &O) const {
----------------
s/annother/another/


================
Comment at: tools/llvm-xray/xray-graph.cc:572
   for (const auto &Record : Trace) {
     // Generate graph, FIXME: better error recovery.
+    auto ER2 = GR.accountRecord(Record);
----------------
Do you need the equivalent of a "keep-going" flag here too (similar to what we have in `llvm-xray account`)?


https://reviews.llvm.org/D28225