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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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.



================
Comment at: llvm/tools/llvm-objcopy/ConfigManager.cpp:697
 
-  CopyConfig Config;
-  Config.InputFilename = Positional[0];
-  Config.OutputFilename = Positional[Positional.size() == 1 ? 0 : 1];
+  ConfigManager Config;
+  Config.Common.InputFilename = Positional[0];
----------------
Optional suggestion, which might help reduce the repetitive `Config.Common` and reduce the changes here:

```
ConfigManager ConfigMgr;
CommonConfig &Confg = ConfigMgr.Common;
```

The same could be done in other places too.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:510
 // system. The only priority is that keeps/copies overrule removes.
-static Error handleArgs(const CopyConfig &Config, Object &Obj) {
-  if (Config.StripSwiftSymbols || Config.KeepUndefined)
-    return createStringError(llvm::errc::invalid_argument,
-                             "option not supported by llvm-objcopy for ELF");
-
-  if (Config.OutputArch) {
-    Obj.Machine = Config.OutputArch.getValue().EMachine;
-    Obj.OSABI = Config.OutputArch.getValue().OSABI;
+static Error handleArgs(const CommonConfig &Common, const ElfConfig &Elf,
+                        Object &Obj) {
----------------
I'd be inclined to call these something like `Config` and `ElfCfg` (or even `ElfConfig` if you don't mind the hiding of the class type within this function). This reduces the changes needed here at least. Thinking about it, even if you want to name `CommonConfig` something reflecting the Common aspect, I'd call the variable `CommonCfg` not simply `Common`, and `Elf` similarly becomes `ElfCfg`. The reason is that `Elf` normally implies some sort of Elf object, not a config class. Renaming `Common` also helps describe its purpose more clearly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99055