[PATCH] D56383: [XRay][tools] Use symbols instead of function id

Kristina Brooks via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jan 7 21:41:04 PST 2019


kristina added a comment.

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



================
Comment at: llvm/tools/llvm-xray/xray-graph-diff.cpp:353
+            .str());
   default:
     if (containsNullptr(VertexAttr.CorrVertexPtr))
----------------
  - 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.

 



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56383/new/

https://reviews.llvm.org/D56383





More information about the llvm-commits mailing list