[PATCH] D118193: [llvm-nm] add a new option --bit-mode 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/XCOFF/option-bit-mode.test:20
+# ANY:        [[FILE32]]:
+# ANY-NEXT:   00000000 D var32
+# ANY:        [[FILE64]]:
----------------
Without FileCheck --match-full-lines, `0000000000000000 D var32` will be matched as well. 


================
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">;
----------------
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.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:88
 enum OutputFormatTy { bsd, sysv, posix, darwin, just_symbols };
+enum BitModeTy { Bit32, Bit64, Any };
 } // namespace
----------------
`enum class`


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1787
+static bool doesNeedToDump(SymbolicFile &Obj) {
+  if ((isSymbolList64Bit(Obj) && BitMode == Bit32) ||
+      (!isSymbolList64Bit(Obj) && BitMode == Bit64))
----------------
Avoid a negative function/variable name which holds a boolean that tells of whether we should not do something. Use the positive form: `shouldDump`

```
if (BitMode == Any)
  return true;
return isSymbolList64Bit(Obj) ? BitMode == Bit64 : BitMode == Bit32
```



================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:2332
+    error(
+        "--bitmode value should be one of: 32,64,any, using default(any) now");
+
----------------
perhaps use this message


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118193