[PATCH] D67139: [llvm-objcopy] Refactor ELF-specific config out to ELFCopyConfig. NFC.
Fangrui Song via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 13 04:11:38 PDT 2019
MaskRay added inline comments.
================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:93
+struct Configurations {
+ const CopyConfig &Common;
+ Optional<ELFCopyConfig> ELF;
----------------
seiya wrote:
> alexshap wrote:
> > frankly speaking I'm not a big fan of this design again, i kinda read James' suggestion differently.
> > Here we have this duplicated reference structure (one reference to Common here, another one is inside ELFConfig) and the responsibilities / concerns of these classes are kinda intricated.
> >
> > If we want to separate Common from everything else - I'm totally fine with that, but would prefer to have a simpler structure here:
> >
> > struct DriverConfig {
> > SmallVector<Configuration, 1> Configurations;
> > BumpPtrAllocator Alloc;
> > };
> >
> > // Parsed command-line arguments
> > struct CopyConfig {
> > ....
> > };
> >
> > // COFF specific details
> > struct MachOCopyConfig {
> > };
> >
> > // ELF-specific details
> > struct ELFCopyConfig {
> > };
> >
> > // MachO specific details
> > struct MachOCopyConfig {
> > };
> >
> > struct Configuration {
> > CopyConfig Common;
> > Optional<ELFCopyConfig> ELF;
> > Optional<MachOCopyConfig> MachO;
> > Optional<COFFCopyConfig> COFF;
> > };
> >
> > Somehow I find it a tiny bit more straightforward / simpler.
> >
> > And have
> > Error executeObjcopyOnObjcopy(const Configuration &Config);
> >
> > etc.
> >
> > CC: @jhenderson
> >
> Eliminating duplicated references to CopyConfig sounds better to me. I'll adopt this design if there are no objections.
I don't have strong opinions on the interface (or I probably should say this really depends on the implementation)
No matter what design this patch ends up with, I hope we can avoid the verbose`Config.Common.Option` and use the simpler `Config.Option`, `Common.Option` or `ELF.Option`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67139/new/
https://reviews.llvm.org/D67139
More information about the llvm-commits
mailing list