[PATCH] D62652: [llvm-objcopy][MachO] Recompute and update offset/size fields in the writer

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:135
+    // Construct a load command.
+    auto MLC = LC.MachOLoadCommand;
+    switch (MLC.load_command_data.cmd) {
----------------
nit: I'd use the explicit type here (for readability)


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:147
+        assert(Sec.Segname.size() <= 16 && "too long segment name");
+        assert(Sec.Sectname.size() <= 16 && "too long section name");
+        memcpy(Temp.segname, Sec.Segname.data(), Sec.Segname.size());
----------------
khm, here and in some other similar places, i'd use sizeof(...)
or create a named constant for this number (16).


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:174
+      for (const auto &Sec : LC.Sections) {
+        struct MachO::section_64 Temp;
+        memset(&Temp, 0, sizeof(struct MachO::section_64));
----------------
here and the lines above (143 - 170) are very similar,
let's create a helper function for this and avoid code duplication.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:438
+void MachOWriter::updateDySymTab(MachO::macho_load_command &MLC) {
+  auto nlocalsym = 0;
+  auto Iter = O.SymTable.NameList.begin();
----------------
explicit type + I think naming here doesn't follow our style guide)


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:448
+
+  auto nextdefsym = 0;
+  for (; Iter != End; Iter++) {
----------------
see the comment on the line 438


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:481
+        auto FilePaddingSize =
+            OffsetToAlignment(FileOffsetInSegment, pow(2, Sec.Align));
+        Sec.Offset = Offset + FileOffsetInSegment + FilePaddingSize;
----------------
   
     1 << Sec.Align
P.S. pow works with floating point numbers, I think we don't really need these conversions here


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

https://reviews.llvm.org/D62652