[PATCH] D119050: [llvm-objdump/mac] Add new function starts print mode

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I'm not a Mach-O person, but I've got some nitpick comments on the general area.



================
Comment at: llvm/docs/CommandGuide/llvm-objdump.rst:359
 
-.. option:: --function-starts
+.. option:: --function-starts=<option>
 
----------------
In other tools (I'm looking at llvm-symbolizer specifically at least), the format would be `--function-starts [=<addrs|names|both>]`, to indicate a) the value is optional, and b) what the possible values are.


================
Comment at: llvm/docs/CommandGuide/llvm-objdump.rst:361
 
-  Print the function starts table for Mach-O objects.
+  Print the function starts table for Mach-O objects. Either ``addrs`` (default), ``names``, or ``both``.
 
----------------
I'm not a Mach-O developer, and so am not particular familair with the object format. Is it obvious to someone who is familiar what the meaning of the addrs/names options actually is?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.h:16
 #include "llvm/Object/Archive.h"
+#include "llvm/Option/Arg.h"
 #include "llvm/Support/Compiler.h"
----------------
I don't think you need the include. You can forward declare `opt::Arg`, I believe?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119050