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

[Keith via Phabricator via llvm-commits]

kpw marked 2 inline comments as done.
kpw added inline comments.


================
Comment at: tools/llvm-xray/xray-stacks.cc:256-258
+  for (auto MapPairIter : LeftCalleesByFuncId) {
+    Node->Callees.push_back(MapPairIter.second);
+  }
----------------
dberris wrote:
> Is it possible to just do something like:
> 
> ```
> auto &RightCallees = MapPairIter.second;
> Node->Callees.insert(Node->Callees.end(), RightCallees.begin(), RightCallees.end());
> ```
> 
> instead?
I think it would be a bit more involved to do it this way and would require llvm::map_iter (has to do with mapping functions to an iterator, not the map type) to extract the values from the pair that iterating over the map presents. This seems less readable to me.


================
Comment at: tools/llvm-xray/xray-stacks.cc:261-263
+  for (auto duration : Left.TerminalDurations) {
+    Node->TerminalDurations.push_back(duration);
+  }
----------------
dberris wrote:
> This one seems simpler as:
> 
> ```
> Node->TerminalDurations.insert(
>   Node->TerminalDurations.end(),
>   Left.TerminalDurations.begin(), Left.TerminalDurations.end());
> ```
> 
> Or, if there's an `append` that takes a range, it might be more efficient/simpler than growing the containers one element at a time.
I don't know of an append that takes a range. That would be nice and I can imagine some SFINAE constructs to detect that the range is a difference iterator and only require one resize.

I didn't make the change to use insert, which imho doesn't activate pattern matchers for iterating through a range in my brain as well as for loops.


Repository:
  rL LLVM

https://reviews.llvm.org/D34863