[PATCH] D59989: [llvm-objcopy]Allow llvm-objcopy to be used on an ELF file with no section headers

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Few nits/questions from me.



================
Comment at: test/tools/llvm-objcopy/ELF/copy-after-strip-sections.test:23
+    Content: 'facefeed'
+    AddressAlign: 0x1000
+    Address: 0x1000
----------------
Is this field important?


================
Comment at: test/tools/llvm-objcopy/ELF/copy-after-strip-sections.test:28
+    VAddr: 0x1000
+    PAddr: 0x1000
+    Sections:
----------------
Do you need `PAddr` for this test?


================
Comment at: tools/llvm-objcopy/ELF/Object.cpp:1573
+    return Obj.SHOffset;
+  size_t ShdrCount = size(Obj.sections()) + 1; // Includes null shdr.
+  return Obj.SHOffset + ShdrCount * sizeof(Elf_Shdr);
----------------
`size` is a bit strange helper. I do not think it is common to use it instead of `.size()` call
for a container in LLVM..
I see it is not your initial code, but since you're changing it, what do you think about not using it here at all?


================
Comment at: tools/llvm-objcopy/ELF/Object.h:233
   ELFWriter(Object &Obj, Buffer &Buf, bool WSH)
-      : Writer(Obj, Buf), WriteSectionHeaders(WSH) {}
+      : Writer(Obj, Buf), WriteSectionHeaders(!Obj.HadShdrs ? false : WSH) {}
 };
----------------
Up to you perhaps, but I would suggest to move it out to the constructor body.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59989