[PATCH] D50117: [llvm-objcopy] NFC: Refactor main objcopy method that takes an ELFReader to a generic Reader.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: tools/llvm-objcopy/Object.cpp:930
+std::unique_ptr<Object> BinaryReader::create() const {
+  error("--input-target Binary is unsupported");
+}
----------------
The aim is to support this at some point in the future, right? If so, I'd say "currently unsupported" to make it clear that it's not a permanent state of affairs necessarily.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:206
 static std::unique_ptr<Writer> CreateWriter(const CopyConfig &Config,
-                                            Object &Obj, Buffer &Buf,
-                                            ElfType OutputElfType) {
+                                            const Reader &Reader, Object &Obj,
+                                            Buffer &Buf) {
----------------
I feel like @jakehehrlich, @alexshap and I had a discussion about this a number of months ago, but I don't really remember what we agreed etc. My current instinct is that a Writer (and the functions used to create it) shouldn't need to know about anything to do with the Reader. This change couples the two together, which I don't really like. I'd rather it be decided outside the CreateWriter function and passed in (as was the case before).

If the problem is the assumption of needing an "ElfType", maybe we should generalise that enum to not be specific to do with ELFs?


Repository:
  rL LLVM

https://reviews.llvm.org/D50117