[PATCH] D114410: [ObjectYAML/obj2yaml/yaml2obj][MachO] Support indirect symbol table

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.

A few nits, but mostly looks good.



================
Comment at: llvm/include/llvm/ObjectYAML/MachOYAML.h:124
   std::vector<StringRef> StringTable;
+  std::vector<llvm::yaml::Hex32> IndirectSymbols;
 
----------------
Shouldn't need the explicit `llvm` prefix, since you're already in the llvm namespace.


================
Comment at: llvm/lib/ObjectYAML/MachOEmitter.cpp:567
+void MachOWriter::writeDynamicSymbolTable(raw_ostream &OS) {
+  for (auto data : Obj.LinkEdit.IndirectSymbols)
+    OS.write(reinterpret_cast<const char *>(&data),
----------------
UpperCamelCase for variable names.

Also, I'd not use `auto` here: the type isn't obvious from the RHS (although if you're keen on following existing style in this immediate area, I can live with it).


================
Comment at: llvm/lib/ObjectYAML/MachOEmitter.cpp:569
+    OS.write(reinterpret_cast<const char *>(&data),
+             sizeof(llvm::yaml::Hex32::BaseType));
+}
----------------



================
Comment at: llvm/test/ObjectYAML/MachO/dsymtab.yaml:3
+
+--- !mach-o
+FileHeader:
----------------
Is this the minimum this document can be reduced to? There's a lot of stuff that has no value to the test, although I know that Mach-O yaml2obj's side doesn't allow much to be removed to minimise noise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114410