[PATCH] D29320: [XRay] A tool for Comparing xray function call graphs
Dean Michael Berris via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 9 00:14:59 PST 2017
dberris requested changes to this revision.
dberris added a comment.
This revision now requires changes to proceed.
Please address all comments by me and @dblaikie
================
Comment at: tools/llvm-xray/xray-graph-diff.cc:222
+
+ for (int i = 0; i < 2; ++i)
+ for (const auto &V : G[i].get().vertices())
----------------
Magic number 2 for a reason? Maybe use the N in GraphDiffRenderer?
================
Comment at: tools/llvm-xray/xray-graph-diff.cc:261
+// which ranges onto [-infty, infty] with the sign corresponding to which number
+// is larger
+static double difference(double A, double B) { return A / B - B / A; }
----------------
Punctuation mark?
================
Comment at: tools/llvm-xray/xray-graph-diff.cc:316
+ GraphDiffRenderer::StatType T) {
+ if (E.second.P[0] == nullptr)
+ return "green";
----------------
What would be really helpful here is if you gave `E.second` a name. Is this an `EdgeAttr`? If so, if would be much easier to read something like:
```
auto &EdgeAttr = E.second;
if (EdgeAttr.Parents[0] == nullptr)
return "green";
if (EdgeAttr.Parents[1] == nullptr)
return "red";
...
EdgeAttr.Parents[0].Stats.getDouble(Stat)
```
================
Comment at: tools/llvm-xray/xray-graph-diff.cc:327-329
+ if (MR == 0.0 && R == 0.0)
+ MR = 1.0;
+
----------------
I'm not an expert here, but comparing to zero seems a little... odd because these are floating point numbers, but maybe not something important enough to worry about.
================
Comment at: tools/llvm-xray/xray-graph-diff.cc:416-418
+ if (A.getPosition() == 0 && AA.getPosition() == 0) {
+ return B;
+ }
----------------
Lose the `{` and `}` ?
================
Comment at: tools/llvm-xray/xray-graph-diff.h:51
+ std::array<std::reference_wrapper<const GraphRenderer::GraphT>,
+ GraphDiffRenderer::N>
+ G;
----------------
Do you need to fully qualify N here?
https://reviews.llvm.org/D29320
More information about the llvm-commits
mailing list