[PATCH] D24377: [XRay] Implement the `llvm-xray account` subcommand

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: tools/llvm-xray/xray-account.cc:174-203
+    if (ThreadStack.back().first != Record.FuncId) {
+      if (!DeduceSiblingCalls)
+        return false;
+      auto Parent =
+          std::find_if(ThreadStack.rbegin(), ThreadStack.rend(),
+                       [&](const std::pair<const int32_t, uint64_t> &E) {
+                         return E.first == Record.FuncId;
----------------
Might be good (maybe not) to invert this to reduce indentation.

  if (first == funcid) {
    ...
    ThreadStack.pop_back();
  }
  if (!DeduceSiblingCalls)
    return false;
  if (Parent == ThreadStack.rend())
    return false;
  while (...) {
    ...
  }
  return true;


================
Comment at: tools/llvm-xray/xray-account.cc:316
+      break;
+    default:;
+      // FIXME: Not supported yet.
----------------
extra semicolon?


================
Comment at: tools/llvm-xray/xray-account.cc:324
+    auto it = Results.begin();
+    std::advance(it, AccountTop.getValue());
+    Results.erase(it, Results.end());
----------------
Probably wouldn't bother usind std::advance unless you're writing a generic algorithm or have a non-random iterator you need to stride forward.

  Results.erase(Results.begin() + AccountTop.getValue(), Results.end());

seems fine.


================
Comment at: tools/llvm-xray/xray-account.cc:332
+
+void LatencyAccountant::exportStatsAsTEXT(raw_ostream &OS,
+                                          const XRayFileHeader &Header) const {
----------------
Text isn't an acronym, so probably shouldn't be capitalized here.


================
Comment at: tools/llvm-xray/xray-account.cc:378-381
+namespace llvm {
+namespace xray {
+
+static CommandRegistration Unused(&Account, []() -> Error {
----------------
Seems a bit weird to put a single static variable in a namespace - maybe just use 'using' decls for the namespace instead?


================
Comment at: tools/llvm-xray/xray-account.cc:395
+              return Error::success();
+            return Error(std::move(E));
+          }))
----------------
No need for "Error(...)" here, this will do, I think:

  return std::move(E);


================
Comment at: tools/llvm-xray/xray-account.cc:414-421
+  switch (AccountInputFormat) {
+  case AccountInputFormats::RAW:
+    Loader = NaiveLogLoader;
+    break;
+  case AccountInputFormats::YAML:
+    Loader = YAMLLogLoader;
+    break;
----------------
It'd be good if reading were refactored into some utility that would just read the input file in whatever format it was in without every tool having to have explicit support for format specifications.


================
Comment at: tools/llvm-xray/xray-account.cc:433
+  for (const auto &Record : Reader) {
+    if (!FCA.accountRecord(Record)) {
+      for (const auto &ThreadStack : FCA.getPerThreadFunctionStack()) {
----------------
reduce indentation:

  if (FCA.accountRecord(Record))
    continue;
  ...


https://reviews.llvm.org/D24377