[PATCH] D54674: [llvm-objcopy] First bits for MachO

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I'd be interested to hear yours and other people's thoughts on not calling reportError inside the MachO layer and propagating it out at least to executeObjcopyOnBinary, via llvm::Error/Expected. I know @jakehehrlich was talking about this in the ELF side at one point too.

FYI, I'm not working a COFF-layer, but it would make a lot of sense to put it in as well (but see my earlier comments about executable naming and option visibility etc).



================
Comment at: test/tools/llvm-objcopy/MachO/real-world-input-32-copy.test:11-12
+
+For the reference, macho-little-endian-32.o can be built on OSX as follows:
+macho.cpp:
+
----------------
Not that it really matters, but it would be nice to add a comment marker or similar here, e.g. '#'.


================
Comment at: test/tools/llvm-objcopy/MachO/real-world-input-64-copy.test:11
+
+For the reference, macho-little-endian-64.o can be built on OSX as follows:
+macho.cpp:
----------------
See above.


================
Comment at: tools/llvm-objcopy/MachO/MachOReader.cpp:11
+#include "MachOReader.h"
+#include "../llvm-objcopy.h"
+#include "Object.h"
----------------
Whilst it is valid, I don't particularly like relative paths in #includes. Is it possible to add an include directory to the top-level of llvm-objcopy, so that you can do `#include "llvm-objcopy.h"` and similarly include the MachO specific stuff as `#include "MachO/SomeFile.h"`


Repository:
  rL LLVM

https://reviews.llvm.org/D54674