[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-gen.wls:1-4
+#!/usr/local/bin/wolframscript
+(* This file generates the arrays in graph-color-helper.cc and the enum types
+  in graph-color-helper.h, run the output through clang-format before copying
+  the definitions output into the c files. *)
----------------
Probably needs top-matter to bear the correct license.


================
Comment at: tools/llvm-xray/xray-color-helper.cc:55
+static const double SequentialCoeffs[][3][21] =
+{{{-6.878460665e6,5.961432639e7,-2.307067298e8,5.170718862e8,-7.138743566e8,
+     5.503115096e8,-3.409350245e7,-4.849896433e8,6.794988641e8,-5.465668628e8,
----------------
Formatting suggestion:

```
static constexpr double Coeffs[][3][21] = {
  { // Describe 2D array
    {  // Describe this array
      -6.878460665e6, ...
       ...
    },
    {  // Describe this array
      -2.54152823e6, ...
      ...
    },
    ...
  },
  ...
};
    


================
Comment at: tools/llvm-xray/xray-color-helper.h:22
+
+class ColorHelper{
+ private:
----------------
This would benefit hugely from documentation. Show how to use it, what to use it for, etc.?


================
Comment at: tools/llvm-xray/xray-color-helper.h:60
+  ColorHelper(DivergingScheme S);
+  std::string getColor(double Point);
+};
----------------
I might be missing something, but would it work better if you returned an RGB tuple?


https://reviews.llvm.org/D29363