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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht marked an inline comment as done.
rupprecht added inline comments.


================
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) {
----------------
jhenderson wrote:
> 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?
> 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. 
I've read through the comments at D41687 (Jake pointed me to that one as a starter for this change), but let me know if there's some other discussion thread you had that I should read first.

> 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).
That's true for CreateWriter itself, but currently ExecuteElfObjcopyOnBinary (which is constructs a writer via CreateWriter) has to look at the ELFReader it has created so it knows what elf type to pass.

Also -- this problem could be pushed back up another layer, i.e. Writer could be passed into ExecuteElfObjcopyOnBinary instead of ExecuteElfObjcopyOnBinary calling CreateWriter itself... although there is currently another chain of ExecuteElfObjcopyOnBinary -> HandleArgs -> SplitDWOToFile -> CreateWriter that would also need to be plumbed.

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

Reading from a binary input should still have an elf type, it will just come from the -B command line flag, instead of being inferred from the object file magic header.

However -- refactoring to your suggestion so that CreateWriter doesn't need to know about Reader, I've chosen to change it to Optional<ElfType>. If the input and output are both "binary" (I'm not sure if that's a valid use of objcopy, but it's at least allowed by command line flags), then there is no elf type.


Repository:
  rL LLVM

https://reviews.llvm.org/D50117