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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc marked an inline comment as done.
pcc 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:
> 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.
I decided to go with Optional<StringRef> in a field of ReaderConfig.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62364