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

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo marked 4 inline comments as done.
mstorsjo added inline comments.


================
Comment at: include/llvm/Object/COFF.h:974
   }
+  std::error_code getCOFFHeader(const coff_file_header *&Res) const {
+    Res = COFFHeader;
----------------
jhenderson wrote:
> We seem to be mixing our styles of defining things in headers and in .cpp files for no particular reason. Would you mind defining these in COFF.cpp, please since there are two that already are? (I'd support moving getDosHeader into the COFF.cpp too in that case).
> 
> I could be persuaded instead that the two PE header functions should be moved here.
Sure, I don't mind moving them there. It's indeed quite a bit inconsistent right now, although I don't know if there's some rationale for which ones have been made inline so far.


================
Comment at: test/tools/llvm-objcopy/COFF/basic-copy.test:25
+  (lld currently can leave a whole empty sector inbetween)
+- The actual layout of of the string table (it can be filled linearly,
+  strings can be dedupliated, the table can be optimized by sharing tails
----------------
jhenderson wrote:
> nit: remove the extra "of"
Thanks, amended the patch locally with that fixed.


================
Comment at: tools/llvm-objcopy/COFF/Object.cpp:47
+
+std::unique_ptr<Object> COFFReader::create() const {
+  auto Obj = llvm::make_unique<Object>();
----------------
jhenderson wrote:
> This is quite a big function. Would it be feasible to split it up into a few helpers?
I guess it should be feasible, I'll give it a try.


================
Comment at: tools/llvm-objcopy/COFF/Object.h:30
+
+class Object;
+
----------------
jhenderson wrote:
> To avoid confusion, maybe we should rename the ELF Object to ELFObject, and then this Object to COFFObject, etc?
Sure, I don't mind doing that.

The same goes for D54674 and MachO as well.

Although that also moves the names closer to the class names used by llvm/Object, `COFFObjectFile` and `ELFObjectFile`, but I don't think that's an issue.


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

https://reviews.llvm.org/D54939