Page MenuHomePhabricator

[XRay][tools] Use symbols instead of function id
Changes PlannedPublic

Authored by dberris on Jan 7 2019, 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.Jan 7 2019, 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.

Gentle ping for author, has this landed or still in the backlog? (If it's landed can you attach the commit and close it, please? Just trying to to avoid diffs on Phab going stale.)

Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2019, 4:35 AM
dberris planned changes to this revision.Feb 27 2019, 2:38 PM

Gentle ping for author, has this landed or still in the backlog? (If it's landed can you attach the commit and close it, please? Just trying to to avoid diffs on Phab going stale.)

Sorry for the delay, this is still worth doing and this is still broken. I'll update this with an end to end test to ensure we have it fixed and won't regress. I've had some recent changes with my work situation that's making this less of a priority for me, hence the silence.