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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/ConfigManager.h:26
+// format-specific options.
+struct ConfigManager : public MultiFormatConfig {
+  virtual ~ConfigManager() {}
----------------
avl wrote:
> dblaikie wrote:
> > Why is there an inheritance relationship here - if there's only one class that implements MultiFormatConfig - why isn't ConfigManager a non-virtual API?
> MultiFormatConfig interface is used when passed to routine working with archives:
> 
> ```
> Expected<std::vector<NewArchiveMember>>
> createNewArchiveMembers(const MultiFormatConfig &Config,
>                         const object::Archive &Ar);
> ```
> 
> It would be bad to pass whole ConfigManager here since ConfigManager has elements not used by createNewArchiveMembers.  Additionally ConfigManager is a llvm-objcopy specific thing. For some another tool(llvm-another-tool) there might be another implementation of config manager, but it needs to be presented as a universal interface for createNewArchiveMembers. MultiFormatConfig is a such interface.
It seems like an abstraction without a use case yet, so it should wait until there is a use case?

I understand some of this is refactoring to make this more amenable to be a general purpose library - but if all the uses of the archive support currently want a big multi-format config, it seems OK for that to be a single non-virtual thing at the moment. Then when someone has a need to build a format-specific tool with archive support (hopefully we don't do too much of that in LLVM - it's nice to be generic over all our supported object formats where possible) they could add this abstraction?

(what's the idea, that they could implement ConfigManager with a version that returns errors for everything other than the object format they support (ie: an ELF-only use of this would return a real ELF config, but getCOFF/MachO/WasmConfig would return Error about how those formats aren't supported)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99055