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

Mon Apr 26 12:08:49 PDT 2021

dblaikie added a comment.

In D99055#2716017 <https://reviews.llvm.org/D99055#2716017>, @jhenderson wrote:

> I've skimmed bits of this briefly for now - I am quite busy and don't have the time for a more in-depth review at this point. It would be really helpful to get another pair of eyes on this approach too, to see what others think.

I've been watching, but my understanding is limited, and from what I've seen it doesn't seem like it's been that beneficial to try to split these things up when they all have to be known to the frontend anyway. So one big struct with some comments about which bits are for which architecture sounds like it would be OK to me - but perhaps I'm missing some things.

================
Comment at: llvm/tools/llvm-objcopy/ConfigManager.h:26
+// format-specific options.
+struct ConfigManager : public MultiFormatConfig {
+  virtual ~ConfigManager() {}
----------------
Why is there an inheritance relationship here - if there's only one class that implements MultiFormatConfig - why isn't ConfigManager a non-virtual API?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99055