[PATCH] D29363: [XRAY] A Color Choosing helper for 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-color-helper.cc:39
+     // http://colorbrewer2.org/
+     {-6.878460665e6, 5.961432639e7,  -2.307067298e8, 5.170718862e8,
+      -7.138743566e8, 5.503115096e8,  -3.409350245e7, -4.849896433e8,
----------------
Is this first polynomial for the "red" component? If so, document that clearly here (both to clearly mark the start of the array, and as an indentation guide).


================
Comment at: tools/llvm-xray/xray-color-helper.cc:398
+// Diverging Polynomials
+
+static const double DivergingCoeffs[][3][21] = {
----------------
Explain what "Diverging Polynomials" means. Really this whole file shouldn't assume that readers will have time to look at the mentioned website just to understand what the names/mnemonics/short-cuts mean.


================
Comment at: tools/llvm-xray/xray-color-helper.cc:581-589
+std::string ColorHelper::getColor(double point) {
+  point = std::max(std::min(point, MaxIn), MinIn);
+
+  uint8_t r = uintIntervalTo8bitChar(polyEval(RedPoly, point));
+  uint8_t g = uintIntervalTo8bitChar(polyEval(GreenPoly, point));
+  uint8_t b = uintIntervalTo8bitChar(polyEval(BluePoly, point));
+
----------------
Reading this, it looks like you do just want to give a `tuple<uint8_t, uint8_t, uint8_t>` as the result, then the caller can transform it where they need it. Right now it's assuming that the caller will always want the string value rather than the triple, which seems like a bad assumption to make.


https://reviews.llvm.org/D29363