[PATCH] D99055: [llvm-objcopy] Refactor CopyConfig structure.

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

> What do you think?

This solution has some concerns caused by lazy options parsing.
I do not understand whether/why we need lazy parsing.
Thus, depending on the reason for lazy parsing, these concerns might be acceptable price or might be not. To support lazy parsing it is necessary 
to keep unparsed data, move some parsing functionality into parseELFConfig(). 
In case it would be necessary to create another tool(llvm-another-tool) then
we need to implement a child of ConfigManager and implement another version of 
parseELFConfig(). All this is doable but looks like unnecessary complications.
If it would be not necessary to support lazy parsing then the solution would look simpler. 
Do we need to support lazy parsing?

The same solution without lazy parsing looks simpler:

  class ConfigManager {
  public:
    ConfigManager(/*...*/); // initializes all options into parsed state.
  
    const CommonConfig &getConfig() const { return Common; }
    const ELFConfig &getELFConfig() const { return Elf; }
    /* Same for other formats. */
    
  private:
    CommonConfig Common;
    COFFConfig   Coff;
    ELFConfig    Elf;
    MachOConfig  MachO;
    WasmConfig   Wasm;
  };
  
  static Error executeObjcopyOnBinary(ConfigManager &Configs, object::Binary &In,
                                      raw_ostream &Out) {
    if (auto *ELFBinary = dyn_cast<object::ELFObjectFileBase>(&In))
      return elf::executeObjcopyOnBinary(Configs.getCommon(), Configs.getELFConfig(), *ELFBinary, Out);
    /* etc */
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99055