[PATCH] D63309: [llvm-objcopy][MachO] Rebuild the symbol/string table in the writer

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've got several stylistic comments, but don't have time to read up on Mach-O right now to review that aspect of it.



================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:124
+void MachOWriter::updateSymbolIndexes() {
+  auto Index = 0;
+  for (auto &Symbol : O.SymTable.Symbols) {
----------------
This shouldn't be auto.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:275
+  char *SymTable = (char *)B.getBufferStart() + SymTabCommand.symoff;
+  for (auto Iter = O.SymTable.Symbols.begin(); Iter != O.SymTable.Symbols.end();
+       Iter++) {
----------------
Standard LLVM style says that you should put the end() evaluation in the initializer part of a for loop, unless it can change:

`for (auto Iter = O.SymTable.Symbols.begin(), End = O.SymTable.Symbols.end(); Iter != End; Iter++)`


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:277
+       Iter++) {
+    std::unique_ptr<SymbolEntry> &Sym = *Iter;
+    auto Nstrx = O.StrTableBuilder.getOffset(Sym->Name);
----------------
Can you just make this a raw pointer and do `Iter->get()`?


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:561
+void MachOWriter::constructStringTable() {
+  for (auto &Sym : O.SymTable.Symbols)
+    O.StrTableBuilder.add(Sym->Name);
----------------
Maybe too much auto here.


================
Comment at: llvm/tools/llvm-objcopy/MachO/Object.h:88
+  std::string Name;
+  int Index;
   uint8_t n_type;
----------------
Is `int` the right type here? It seems dubious to me without knowing anything about the file format.


================
Comment at: llvm/tools/llvm-objcopy/MachO/Object.h:113
+  bool Scattered;
+  struct MachO::any_relocation_info Info;
+};
----------------
Don't need `struct` here I believe.


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

https://reviews.llvm.org/D63309