[PATCH] D29320: [XRay] A tool for Comparing xray function call graphs

[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.

Overall needs formatting changes, please use clang-format using the style defined in the LLVM codebase.



================
Comment at: tools/llvm-xray/xray-graph-diff.cc:236-266
+  for (const auto &V : G1.vertices())
+    R.G[V.second.SymbolName].A = &V;
+
+  for (const auto &V : G2.vertices())
+    R.G[V.second.SymbolName].B = &V;
+
+  for (const auto &E : G1.edges()){
----------------
I would really much prefer that we limit the repetition here for readability's sake. Factor the logic out into a single function, and provide the graphs as inputs?


================
Comment at: tools/llvm-xray/xray-graph-diff.cc:285-287
+static double divF(double A, double B){
+  return A/B - B/A;
+}
----------------
This deserves a comment I think.


================
Comment at: tools/llvm-xray/xray-graph-diff.cc:391
+
+  OS << "digraph xray {\n";
+
----------------
Maybe give a different name for the diff graph?


================
Comment at: tools/llvm-xray/xray-graph-diff.h:10-11
+//
+// Generate a DOT file to represent the function call graph encountered in
+// the trace.
+//
----------------
Probably needs to be updated.


================
Comment at: tools/llvm-xray/xray-graph.h:53
+    TimeStat(): TimeStat(0,0,0,0,0,0,0){};
+    TimeStat(int64_t _Count,
+             double _Min,
----------------
Please don't use names that start with _.


================
Comment at: tools/llvm-xray/xray-graph.h:62
+      Max(_Max), Sum(_Sum){}
+
+    std::string getString(StatType T) const;
----------------
Now that you've defined a default constructor and a non-default constructor, you might as well define (or =default) a copy constructor, a move constructor, a copy+move assignment (probably the unified assignment operator), a swap overload, etc.

Can you say instead why you needed to do this now?


https://reviews.llvm.org/D29320