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

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added a comment.

In general I think this could do with a bit more documentation, about the semantics of what we're trying to do (as opposed to how we're doing it).



================
Comment at: tools/llvm-xray/xray-graph.cc:170
+                                       cl::desc("Alias for -edge-label"),
+                                       cl::sub(Graph));
+template <class T> static T diff(T L, T R) {
----------------
Empty line after this?


================
Comment at: tools/llvm-xray/xray-graph.cc:171
+                                       cl::sub(Graph));
+template <class T> static T diff(T L, T R) {
+  return std::max(L, R) - std::min(L, R);
----------------
This is a template, no need to be static (it's implicitly inline).


================
Comment at: tools/llvm-xray/xray-graph.cc:175
+
+static void updateStat(GraphRenderer::TimeStat &S, int64_t lat) {
+  S.Count++;
----------------
Style guide says identifiers ought to have an uppercase start letter.


================
Comment at: tools/llvm-xray/xray-graph.cc:184-187
+// Evaluates an XRay record and performs accounting on it, creating and
+// decorating a function call graph as it does so. It does this by maintaining
+// a call stack on a per-thread basis and adding edges and verticies to the
+// graph as they are seen for the first time.
----------------
Can we describe this in less round-about way?

We can spend a few words in documentation to indicate what the side-effects are and some detail of what the algorithm being implemented is.


================
Comment at: tools/llvm-xray/xray-graph.cc:192-193
+//
+// FIXME: make more robust to errors and
+// Decorate Graph More Heavily.
+// FIXME: Refactor this and account subcommand to reduce code duplication.
----------------
Was the intent to use `llvm::Expected<...>`? In which case I'd prefer you change this now.


https://reviews.llvm.org/D28225