[PATCH] D67139: [llvm-objcopy] Refactor ELF-specific config out to ELFCopyConfig. NFC.

[Seiya Nuta via Phabricator via llvm-commits]

seiya added a comment.

In D67139#1657802 <https://reviews.llvm.org/D67139#1657802>, @jhenderson wrote:

> I've got another suggestion: have separate COFFConfig/ELFConfig etc classes that only contain the interpreted versions and lazily construct them prior to calling the corresponding code path. CopyConfig would contain the raw unparsed strings (perhaps rename them to "RawSymbolsToAdd" etc). Perhaps a higher-level Config class could exist just to wrap them and pass them to executeObjcopyOnBinary etc.
>
> Rough outline:
>
> CopyConfig.h:
>
>   class CopyConfig {
>     std::vector<StringRef> RawSymbolsToAdd;
>     ...
>   };
>  
>   class ELFConfig {
>     std::vector<NewSymbolInfo> SymbolsToAdd;
>     ...
>   };
>  
>   // etc for COFF, Mach-O...
>
>
> llvm-objcopy.cpp:
>
>   struct Configurations {
>     CopyConfig Common;
>     Optional<ELFConfig> ELF;
>     ...
>   }
>  
>   ...
>  
>   static Error executeObjcopyOnBinary(Configurations &Cfgs, ...) {
>     if (/*isElf*/...) {
>       if (!Cfgs.ELF)
>        Cfgs.ELF.emplace(Cfgs.Common);
>       return elf::executeObjcopyOnBinary(Cfgs.Common, Cfgs.ELF, ...)
>     }
>     ...
>   }
>


LGTM. I'll implement the idea.

> On a related note, I think the ELF-specific etc parsing code belongs in a file other than ELFObjcopy.cpp. I feel like that file should be confined to doing the actual objcopy processing, whereas the command-line option parsing etc should be separate, e.g. in a new file called ELFConfig.cpp etc. What do you think?

I agree with this. I'll add ELFConfig.cpp to separate the command-line parsing.


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

https://reviews.llvm.org/D67139