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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added a comment.

I've looked at the code, there are few minor nits, but to me this code looks like a pretty good start, many thanks for working on this ))



================
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),
----------------
nit: i'd probably replace the names A and B with smth that reflects they are the symbol types


================
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;
----------------
I would add a comment saying that it's on purpose (that the indices start with 1)


================
Comment at: tools/llvm-objcopy/COFF/Reader.h:32
+class COFFReader : public Reader {
+private:
+  const COFFObjectFile &COFFObj;
----------------
nit: don't need private here


================
Comment at: tools/llvm-objcopy/COFF/Reader.h:40
+public:
+  std::unique_ptr<Object> create() const override;
+  explicit COFFReader(const COFFObjectFile &O) : COFFObj(O) {}
----------------
nit: for consistency with the code above I would swap these two lines 
(first - ctor, second - create)


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

https://reviews.llvm.org/D54939