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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Needs test coverage for the color functionality, I think? (I don't see any colors being tested in the tests)



================
Comment at: tools/llvm-xray/xray-graph.cc:264-270
+  M.Count  = std::max(M.Count,  S.Count);
+  M.Min    = std::max(M.Min,    S.Min);
+  M.Median = std::max(M.Median, S.Median);
+  M.Pct90  = std::max(M.Pct90,  S.Pct90);
+  M.Pct99  = std::max(M.Pct99,  S.Pct99);
+  M.Max    = std::max(M.Max,    S.Max);
+  M.Sum    = std::max(M.Sum,    S.Sum);
----------------
Could use a member pointer & a helper function (possibly template) to maybe make these shorter/more reliable (by only having to mention the member name once - less chance of accidentally mismatching them).


================
Comment at: tools/llvm-xray/xray-graph.cc:310
+namespace {
+// A Helper functio for Normalise Statistics which normalises a single
+// TimeStat element.
----------------
Typo, 'function'

Also probably s/Normalise Statistics/normaliseStatistics/

Also, normalize rather than normalise (consistency with the rest of the codebase)


================
Comment at: tools/llvm-xray/xray-graph.cc:419-442
+  case GraphRenderer::StatType::COUNT:
+    retval = static_cast<double>(Count)/static_cast<double>(O.Count);
+    break;
+  case GraphRenderer::StatType::MIN:
+    retval = Min/O.Min;
+    break;
+  case GraphRenderer::StatType::MED:
----------------
Could use another member pointer helper to handle this table, if you like.


https://reviews.llvm.org/D28225