[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 a comment.
This revision now requires changes to proceed.

Almost there...



================
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,
----------------
dberris wrote:
> 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).
I hate to do this again, but this would look better if the comment was the same line as the `{` and the numbers were indented after the brace. Like so:

```
  { // Red coefficients
    -6.878...,
  },
  { // Green coefficients
    -2.545...,
  },
```


================
Comment at: tools/llvm-xray/xray-color-helper.cc:632-638
+ColorHelper::ColorHelper(ColorHelper::DivergingScheme S) {
+  MinIn = -1.0;
+  MaxIn = 1.0;
+  RedPoly = DivergingCoeffs[static_cast<int>(S)][0];
+  GreenPoly = DivergingCoeffs[static_cast<int>(S)][1];
+  BluePoly = DivergingCoeffs[static_cast<int>(S)][2];
+}
----------------
Does the initialisation have to happen in the constructor body, or could some (or all) of these go on an initialiser list instead?


================
Comment at: tools/llvm-xray/xray-color-helper.cc:665
+// color as a tuple of color components.
+std::tuple<uint8_t, uint8_t, uint8_t> ColorHelper::getColorTuple(double Point) {
+  Point = std::max(std::min(Point, MaxIn), MinIn);
----------------
Should also probably be a const member function.


================
Comment at: tools/llvm-xray/xray-color-helper.cc:677
+// string.
+std::string ColorHelper::getColorString(std::tuple<uint8_t, uint8_t, uint8_t> t) {
+  return llvm::formatv("#{0:X-2}{1:X-2}{2:X-2}",
----------------
Should probably be a const member function.


https://reviews.llvm.org/D29363