[PATCH] D62364: llvm-objcopy: Implement --extract-partition and --extract-main-partition.

Jake Ehrlich via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri May 31 14:58:46 PDT 2019

jakehehrlich added inline comments.

Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:734
                              object::ELFObjectFileBase &In, Buffer &Out) {
-  ELFReader Reader(&In);
+  ELFReader Reader(&In, Config);
   std::unique_ptr<Object> Obj = Reader.create();
jakehehrlich wrote:
> pcc wrote:
> > jakehehrlich wrote:
> > > Adding the CopyConfig here is antithetical to the goal of making this code into a library at some point in the future. We should find a way to avoid this. This goes for propagation of Config elsewhere in the code as well. Propogation of CopyConfig is a hard no from me personally.
> > In an earlier version of the code [1] I was just passing down the name of the partition to extract. Would that work better (maybe with the name passed as a ctor argument rather than as a field)? Alternatively, we could create something like a ReaderConfig to be passed down here that for the moment would just contain the partition name.
> > 
> > [1] https://github.com/pcc/llvm-project/blob/c9939239d7e829e2c779138aaedea061f4a095d0/llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp#L597
> Either ReaderConfig or just passing the partition name makes sense to me.
I think using Option<StringRef> or having two constructors, one with and one without would be best actully. That way you can distinguish between the case for not extracting main and extracting main.

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list