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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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, ...)
    }
    ...
  }

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?


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

https://reviews.llvm.org/D67139