[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;
----------------
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



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

https://reviews.llvm.org/D67139