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

[Seiya Nuta via Phabricator via llvm-commits]

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


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:93
+struct Configurations {
+  const CopyConfig &Common;
+  Optional<ELFCopyConfig> ELF;
----------------
alexshap wrote:
> frankly speaking I'm not a big fan of this design again, i kinda read James' suggestion differently.
> Here we have this duplicated reference structure (one reference to Common here, another one is inside ELFConfig) and the responsibilities / concerns of these classes are kinda intricated.
> 
> If we want to separate Common from everything else - I'm totally fine with that, but would prefer to have a simpler structure here:
>   
>   struct DriverConfig {
>       SmallVector<Configuration, 1> Configurations; 
>       BumpPtrAllocator Alloc;
>   };
>   
>   // Parsed command-line arguments
>   struct CopyConfig {
>        ....
>   };
>   
>   // COFF specific details
>   struct MachOCopyConfig {
>   };
> 
>   // ELF-specific details
>   struct ELFCopyConfig {
>   };
> 
>   // MachO specific details
>   struct MachOCopyConfig {
>   };
>   
>   struct Configuration {
>       CopyConfig Common;
>       Optional<ELFCopyConfig> ELF;
>       Optional<MachOCopyConfig> MachO;
>       Optional<COFFCopyConfig> COFF;
>   };
>   
> Somehow I find it a tiny bit more straightforward / simpler.
> 
> And have 
>   Error executeObjcopyOnObjcopy(const Configuration &Config);
> 
> etc. 
> 
> CC: @jhenderson
> 
Eliminating duplicated references to CopyConfig sounds better to me. I'll adopt this design if there are no objections.


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

https://reviews.llvm.org/D67139