[PATCH] D34863: [XRay][tools] Function call stack based analysis tooling for XRay traces

[Keith via Phabricator via llvm-commits]

kpw added inline comments.


================
Comment at: tools/llvm-xray/xray-stacks.cc:330-347
+          {
+            auto E = std::make_pair(Top, TopSum);
+            TopStacksBySum.insert(std::lower_bound(TopStacksBySum.begin(),
+                                                   TopStacksBySum.end(), E,
+                                                   greater_second),
+                                  E);
+            if (TopStacksBySum.size() == 11)
----------------
dblaikie wrote:
> Not sure these explicit scopes ("{}") are sufficiently valuable (reducing the scope of 'E') to worry about/include?
Ack.


================
Comment at: tools/llvm-xray/xray-stacks.cc:374
+
+static CommandRegistration Unused(&Stack, []() -> Error {
+  // Load each file provided as a command-line argument. For each one of them
----------------
dblaikie wrote:
> Could potentially write this as "[] {" I think, not sure if that's more readable though.
> 
> ("()" can be omitted in a lambda, as can the return type if it's deducible from the return expressions consistently - at least I think that's supported on LLVM's supported compilers)
Because of implicit conversion from llvm::ErrorSuccess to llvm::Error, the return type deduction fails. I also believe the return type aids reading.


================
Comment at: tools/llvm-xray/xray-stacks.cc:393
+    }
+    }
+    return AccountRecordStatus::OK;
----------------
dberris wrote:
> Do you need a case for tail exits? You might also want to log/ignore other kinds of records.
Xray record doesn't have tail exits yet. This is now an error type in account record that gets logged though and a TODO.


================
Comment at: tools/llvm-xray/xray-stacks.cc:432-434
+    std::for_each(
+        map_iterator(Roots.begin(), SecondFn),
+        map_iterator(Roots.end(), SecondFn),
----------------
dberris wrote:
> Is this something you can turn into a `llvm::transform` instead?
I couldn't work out how to map it onto llvm::transform or std::transform. The crux of the trouble is that the contents of each pair in the map's iterator would have to be "flattened" before assignment to the output iterator. Maybe there is something in <algorithm> or STLExtras.h for this, but std::for_each with a back_inserter does it well even if it's a bit ugly.


https://reviews.llvm.org/D34863