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

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added inline comments.
This revision is now accepted and ready to land.


================
Comment at: tools/llvm-xray/xray-account.cc:34
+static cl::opt<bool>
+    AccountKeepGoing("keep_going", cl::desc("Keep going on errors encountered"),
+                     cl::sub(Account), cl::init(false));
----------------
Is this necessary/useful?
Is this tested?

Also, I think flags tend to be - not _ separated.


================
Comment at: tools/llvm-xray/xray-account.cc:39-42
+static cl::opt<bool> AccountDeduceSiblingCalls(
+    "deduce-sibling-calls",
+    cl::desc("Deduce sibling calls when unrolling function call stacks"),
+    cl::sub(Account), cl::init(false));
----------------
Test coverage?


================
Comment at: tools/llvm-xray/xray-account.cc:47-69
+static cl::opt<std::string>
+    AccountOutput("output", cl::value_desc("output file"), cl::init("-"),
+                  cl::desc("output file; use '-' for stdout"),
+                  cl::sub(Account));
+static cl::alias AccountOutput2("o", cl::aliasopt(AccountOutput),
+                                cl::desc("Alias for -output"),
+                                cl::sub(Account));
----------------
Having to specify both input files, as well as input and output formats every time seems a bit tedious. Could we detect formats using file magic & perhaps have some way of unifying the two files (the map and the log) in common/some cases?


================
Comment at: tools/llvm-xray/xray-account.cc:219-223
+    auto I = std::next(Parent).base();
+    std::for_each(I, ThreadStack.end(),
+                  [this, &Record](const std::pair<const int32_t, uint64_t> &E) {
+                    recordLatency(E.first, diff(E.second, Record.TSC));
+                  });
----------------
Might be easier written/read as:

  for (auto &a : make_range(I, ThreadStack.end()))
    ...
?


================
Comment at: tools/llvm-xray/xray-account.cc:221
+    std::for_each(I, ThreadStack.end(),
+                  [this, &Record](const std::pair<const int32_t, uint64_t> &E) {
+                    recordLatency(E.first, diff(E.second, Record.TSC));
----------------
Capture all by reference for any lambda that doesn't escape its own scope (ie: if the lambda isn't put in a std::function or equivalent, feel free to [&] )


================
Comment at: tools/llvm-xray/xray-account.cc:297-300
+      std::sort(Results.begin(), Results.end(),
+                [](const TupleType &L, const TupleType &R) {
+                  return std::get<0>(L) < std::get<0>(R);
+                });
----------------
Might be worth a helper function template that takes the non-type template argument to sort by.


================
Comment at: tools/llvm-xray/xray-account.cc:399
+
+static CommandRegistration Unused(&Account, []() -> Error {
+  int Fd;
----------------
Can skip '()' here, possibly skip '-> Error' too, if you like (see below)


================
Comment at: tools/llvm-xray/xray-account.cc:410
+  if (auto E = handleErrors(
+          std::move(Err), [&](std::unique_ptr<StringError> E) -> Error {
+            if (E->convertToErrorCode() == std::errc::no_such_file_or_directory)
----------------
Do you need the "-> Error" there? (I know the "if every return expression deduces to the same type it can be left implicit" was accepted as a DR to C++11 but not sure it's in all the compilers we support)

(same for the lambda being passed to the Unused variable's ctor too)


================
Comment at: tools/llvm-xray/xray-log-reader.h:49
 
-Error NaiveLogLoader(StringRef Data, XRayFileHeader &FileHeader,
-                     std::vector<XRayRecord> &Records);
-Error YAMLLogLoader(StringRef Data, XRayFileHeader &FileHeader,
-                    std::vector<XRayRecord> &Records);
+enum class LogInputFormats { RAW, YAML };
+LogReader::LoaderFunction getSupportedLoader(LogInputFormats Format);
----------------
Should this perhaps be called "LogInputFormat" (singular rather than plural)?


https://reviews.llvm.org/D24377