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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I'm back from my time off, although have a massive backlog to work my way through, so haven't had too much time to revisit the history of this and related patches, so far. I took a step back and was wondering whether another approach would be significantly simpler. There's likely problems with my below suggesting, which may have already been covered elsewhere, but hopefully any explanations of why any issues are indeed issues will help me understand things better. Apologies if you've addressed some of the issues before.

Basically, the idea is to change the executeObjcopyOn* functions to take two separate configs, a base one and a format-specific one. Each file format would (where necessary) have a simple struct that contains the format-specific values, and then the base one would have the options shared by more than one format. This would of course require updating all references to the `Config` to reference the correct version as required, but if it leads to a nicer design overall, I think it is worth it.

To assist with the lazy parsing, a factory-like class could be responsible for providing the appropriate config when requested. The code might like something like this:

  class ConfigManager {
  public:
    ConfigManager(/*...*/); // Constructs Common. Stores raw argument data needed to construct other configs.
  
    const CommonConfig &getConfig() const { return Common; }
    const ELFConfig &getELFConfig() {
      if (!Elf)
        Elf = ...;
      return *Elf;
    }
    /* Same for other formats. */
    
  private:
    CommonConfig Common;
    Optional<COFFConfig> Coff;
    Optional<ELFConfig> Elf;
    Optional<MachOConfig> MachO;
    Optional<WasmConfig> Wasm;
  };
  
  static Error executeObjcopyOnBinary(ConfigManager &Configs, object::Binary &In,
                                      raw_ostream &Out) {
    if (auto *ELFBinary = dyn_cast<object::ELFObjectFileBase>(&In)) {
      if (Error E = Config.parseELFConfig())
        return E;
      return elf::executeObjcopyOnBinary(Configs.getCommon(), Configs.getELFConfig(), *ELFBinary, Out);
    }
    /* etc */
  }

What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99055