[PATCH] D118193: [llvm-nm] add a new option -X to specify the type of object file llvm-nm should examine

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/test/tools/llvm-nm/option-X-invalid-arg.test:1
+## Test invalid argument for option "-X".
+
----------------
Not necessary to use a separate file for errors. It can be placed at the end of `option-X.test`. I usually place error tests at the end.

I'd avoid `-invalid-arg` in the filename since I may test more than one types of errors and `-invalid-arg` restricts the types of errors the file can test.


================
Comment at: llvm/test/tools/llvm-nm/option-X-invalid-arg.test:5
+
+# RUN: not llvm-nm --format=just-symbols -X33 %t_elf32.o 2>&1 |  FileCheck %s
+
----------------



================
Comment at: llvm/test/tools/llvm-nm/option-X-invalid-arg.test:7
+
+# CHECK:      llvm-nm{{(\.exe)?}}: error: : -X value should be one of: 32, 64, 32_64, (default) any
+# CHECK-NEXT: global_data_symbol32
----------------
It's not necessary to test the filename part. 

Is there a stray colon in `: : `?


================
Comment at: llvm/tools/llvm-nm/Opts.td:16
 
+defm bit_mode : Eq<"bit-mode", "Specifies the type of object file llvm-nm should examine. The bit-mode must be one of : 32, 64 ,any(default)">;
 def debug_syms : FF<"debug-syms", "Show all symbols, even debugger only">;
----------------
DiggerLin wrote:
> jhenderson wrote:
> > MaskRay wrote:
> > > Ensure the space separators are consistently used: `32, 64, (default) any` or `32, 64, any (default)`
> > > 
> > > The leading message can be "(AIX specific) Specify the type of object file to examine". Note that other messages are imperative sentences and don't use the third-person singular. The tool is called `llvm-nm`, so it is unneeded to repeat `llvm-nm` in individual option messages.
> > Marked as done, but not addressed?
> -X is not only AIX specific now. it work for llvm bit code, elf object file,xcoff object file from gnu archive and big archive or from command line input .
Imperative: Specifies -> Specify

`The value must be one of : ` => `The value must be one of: `


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:2343
 
   // XCOFF specific options.
+  StringRef Mode = Args.getLastArgValue(OPT_X, "any");
----------------
Remove `// XCOFF specific options.` since it's no longer XCOFF specific.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118193