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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:93
+struct Configurations {
+  const CopyConfig &Common;
+  Optional<ELFCopyConfig> ELF;
----------------
seiya wrote:
> 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.
I don't have strong opinions on the interface (or I probably should say this really depends on the implementation)

No matter what design this patch ends up with, I hope we can avoid the verbose`Config.Common.Option` and use the simpler `Config.Option`, `Common.Option` or `ELF.Option`.


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

https://reviews.llvm.org/D67139