[PATCH] D54939: [llvm-objcopy] Initial COFF support

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo marked 6 inline comments as done.
mstorsjo added a comment.

Thanks for the review!



================
Comment at: tools/llvm-objcopy/COFF/Object.h:61
+// intermediate data structure.
+template <class A, class B> void copySymbol(A &Dest, const B &Src) {
+  static_assert(sizeof(Dest.Name.ShortName) == sizeof(Src.Name.ShortName),
----------------
alexshap wrote:
> nit: i'd probably replace the names A and B with smth that reflects they are the symbol types
Sure, will replace with Symbol1Ty/Symbol2Ty.


================
Comment at: tools/llvm-objcopy/COFF/Reader.cpp:63
+void COFFReader::readSections(Object &Obj) const {
+  for (size_t I = 1, E = COFFObj.getNumberOfSections(); I <= E; I++) {
+    const coff_section *Sec;
----------------
alexshap wrote:
> I would add a comment saying that it's on purpose (that the indices start with 1)
Good point, will do.


================
Comment at: tools/llvm-objcopy/COFF/Reader.h:32
+class COFFReader : public Reader {
+private:
+  const COFFObjectFile &COFFObj;
----------------
alexshap wrote:
> nit: don't need private here
Thanks, leaving it out - doing the same for COFFWriter as well.


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

https://reviews.llvm.org/D54939