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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/trunk/tools/llvm-xray/xray-graph.cc:212-213
+Error GraphRenderer::accountRecord(const XRayRecord &Record) {
+  using std::make_error_code;
+  using std::errc;
   if (CurrentMaxTSC == 0)
----------------
Should probably be using llvm::errc - see the Support/errc.h - it defines portable constants we know work across all the compilers LLVM supports, etc.


================
Comment at: llvm/trunk/tools/llvm-xray/xray-graph.cc:401-403
+  for (const auto &c : a) {
+    B = c + B * x_0;
+  }
----------------
Usually don't bother with {} on single line scopes.


================
Comment at: llvm/trunk/tools/llvm-xray/xray-graph.cc:586-592
+    for (const auto &ThreadStack : GR.getPerThreadFunctionStack()) {
+      errs() << "Thread ID: " << ThreadStack.first << "\n";
+      auto Level = ThreadStack.second.size();
+      for (const auto &Entry : llvm::reverse(ThreadStack.second))
+        errs() << "#" << Level-- << "\t"
+               << FuncIdHelper.SymbolOrNumber(Entry.FuncId) << '\n';
     }
----------------
Should accountRecord do this work/include this information in its Error (rather than callers doing so)?


================
Comment at: llvm/trunk/tools/llvm-xray/xray-graph.h:135
 
-  /// An enum for enumerating the various statistics gathered on latencies
-  enum class StatType { COUNT, MIN, MED, PCT90, PCT99, MAX, SUM };
+  const PerThreadFunctionStackMap getPerThreadFunctionStack() const {
+    return PerThreadFunctionStack;
----------------
Const value return type? I'm assuming this should be a const ref return type? (we should really have a clang-tidy for that if we don't already, maybe even a straight-up compiler warning)


Repository:
  rL LLVM

https://reviews.llvm.org/D28225