[PATCH] D67139: [llvm-objcopy] Refactor ELF-specific config out to ELFCopyConfig. NFC.
Alexander Shaposhnikov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 13 10:51:29 PDT 2019
alexshap added inline comments.
================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:93
+struct Configurations {
+ const CopyConfig &Common;
+ Optional<ELFCopyConfig> ELF;
----------------
MaskRay wrote:
> 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`.
struct DriverConfig {
SmallVector<Configuration, 1> Configurations;
BumpPtrAllocator Alloc;
};
// COFF specific details
struct MachOCopyConfig {
};
// ELF-specific details
struct ELFCopyConfig {
};
// MachO specific details
struct MachOCopyConfig {
};
struct CopyConfig {
...
Optional<ELFCopyConfig> ELF;
Optional<MachOCopyConfig> MachO;
Optional<COFFCopyConfig> COFF;
};
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67139/new/
https://reviews.llvm.org/D67139
More information about the llvm-commits
mailing list