Page MenuHomePhabricator

[XRay][tools] Use symbols instead of function id
Needs ReviewPublic

Authored by dberris on Mon, Jan 7, 3:44 AM.

Details

Reviewers
mboerger
kristina
Summary

In the graph-diff tool, we should use the symbol name for the labels in
the graph instead of the function ids.

Event Timeline

dberris created this revision.Mon, Jan 7, 3:44 AM
kristina added a subscriber: kristina.

Added some inline comments.

llvm/tools/llvm-xray/xray-graph-diff.cpp
349

Drop the formatv and the raw literal. It's in the old code sure, but formatv implementation in LLVM is rather expensive for two braces and the raw literal here makes zero sense. Use a Twine instead. Same goes for a a few below.

358

truncateString returns a Twine, you're not storing it anywhere so it's still on the stack at that point, what's the point of the explicit conversion here? .str(). A lot of things including formatv can accept a Twine as an argument.

Quite a big style nit. I'd say get rid of the switch completely.

llvm/tools/llvm-xray/xray-graph-diff.cpp
353
  • Default is generally the first case in a switch.
  • Switch case, followed by if, followed by return. Somewhat difficult to parse.
  • No extra indentation for case statements.
  • Is there a case for using a switch statement here at all? I think just an if statement will do, plus you can make use of early return here more easily. Switch is generally hard to parse (by a human) construct so I think it's generally a bad idea to use 2 case switches.