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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

> But also I realize this code review seems like it's gone on/been through so many different fairly expensive variations, that I don't want to come in half-informed and complicate this review further. I'd like to try to simplify/streamline the review a bit if I can help with tie-breakers or other things that might enable progress.
>
> @jhenderson (@rupprecht?) - is there some way we could help settle this work a bit more than it seems to have been able to be so far? (happy to video chat, etc, if that might be helpful)

Sorry, I've been snowed under with other reviews, combined with workloads elsewhere, so I haven't been able to keep focused on this patch and given it the time it needs. It's also meant that as I've come back to it every so often, I see things differently, hence the occasional changes in opinion.

FWIW, I think the current design generally looks reasonably good to me, but I'm open to other design ideas that satisfy the goals of both the new use-case and the existing behaviour. If an alternative looks simpler/more effective, please feel free to shout, though it might be easier to write it in a different patch so that we can compare and contrast competing approaches.

@avl, when I meant see a use-case, I meant literally see a patch that makes use of this refactoring in its current form. If a later patch can't be cleanly based on this patch, then it indicates a problem with the current design.

Aside: the big `if (/*various option checks*/` for the file formats is always something that's bothered me, and I wonder if we really need them. Perhaps that's a different conversation to be had, but it feels like not having them would somewhat simplify some of this code too.



================
Comment at: llvm/tools/llvm-objcopy/ConfigManager.cpp:1047
   DriverConfig DC;
-  CopyConfig Config;
+  ConfigManager Config;
   InstallNameToolOptTable T;
----------------
Same here as above - use `Config` to refer to the `CommonConfig` to save having to rename everything. Same goes thoughout this patch.


================
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];
----------------
jhenderson wrote:
> 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.
I specifically meant the name `Config` in my example (well, with a typo...), as it stops you needing to constantly change `Config` to `CommonCfg` in this function. The same sort of thing goes throughout this patch. In other words, `Config` would represent the common stuff, and then `ELFConfig` etc would represent the format-specific stuff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99055