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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:575
 
-static void ExecuteElfObjcopyOnBinary(const CopyConfig &Config, Binary &Binary,
+static void ExecuteElfObjcopyOnBinary(const CopyConfig &Config, Reader &Reader,
                                       Buffer &Out) {
----------------
rupprecht wrote:
> jakehehrlich wrote:
> > This change (which is not necessarily wrong) conflicts with one of the two proposals I made for how --dump-section should work in another change. After a bit of thought I think "ExecuteElfObjcopyOnBlob" might be needed or something. The --dump-section change hasn't really shaken out just yet so we'll see.
> I renamed this to ExecuteElfObjcopyOnBlob since it's no longer always a binary -- is that what you meant?
> Do you have a link to your proposals on --dump-section?
No I meant for ExecuteElfObjcopyOnBinary to remain unchanged and to not take a Reader at all but instead for a separate almost identical function  ExecuteElfObjcopyOnBlob to exist so that current invocations to ExecuteElfObjcopyOnBinary would have to decide which one to call. The issue is that sometimes there are options (like --dump-section) which only really make sense on the original data. Depending on how that all shakes out we might want to handle that option differently depending on if -I binary or -I elf* was used. One of the options (and the option I'm currently in favor of) for that change would allow us to treat this all uniformly however.


Repository:
  rL LLVM

https://reviews.llvm.org/D50117