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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:93
+struct Configurations {
+  const CopyConfig &Common;
+  Optional<ELFCopyConfig> ELF;
----------------
MaskRay wrote:
> 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`.

  struct DriverConfig {
    SmallVector<Configuration, 1> Configurations; 
    BumpPtrAllocator Alloc;
  };
  
  // COFF specific details
  struct MachOCopyConfig {
  };

  // ELF-specific details
  struct ELFCopyConfig {
  };

  // MachO specific details
  struct MachOCopyConfig {
  };

  struct CopyConfig {
     ...
     Optional<ELFCopyConfig> ELF;
     Optional<MachOCopyConfig> MachO;
     Optional<COFFCopyConfig> COFF;
  };


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

https://reviews.llvm.org/D67139