[PATCH] D81614: [llvm][llvm-nm] add TextAPI/MachO support

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added inline comments.


================
Comment at: llvm/include/llvm/Object/TapiFile.h:57
   std::vector<Symbol> Symbols;
+  MachO::Architecture SupportedArch;
 };
----------------
Why not just `Arch` like the ctor argument? 


================
Comment at: llvm/include/llvm/TextAPI/MachO/Architecture.h:50
+/// Check if architecture is 64 bit
+bool is64Bit(Architecture);
+
----------------
Would `getNumberOfBits` be a more "generic" API? 


================
Comment at: llvm/lib/Object/TapiFile.cpp:44
     : SymbolicFile(ID_TapiFile, Source) {
+  SupportedArch = Arch;
   for (const auto *Symbol : interface.symbols()) {
----------------
Why not do this in the initializer list?


================
Comment at: llvm/lib/TextAPI/MachO/Architecture.cpp:91
+  // switch statement.
+  return false;
+}
----------------
`llvm_unreachable("Fully covered switch above!");`


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:2111
+      auto ObjOrErr = I.getAsObjectFile();
+      if (ObjOrErr) {
+        auto &Obj = *ObjOrErr.get();
----------------
I think you should be able to do:
```
if (auto ObjOrErr = I.getAsObjectFile())
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81614