[PATCH] D50117: [llvm-objcopy] NFC: Refactor main objcopy method that takes an ELFReader to a generic Reader.

Jordan Rupprecht via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Aug 2 13:01:08 PDT 2018


rupprecht added inline comments.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:291
 
+static Optional<ElfType> GetOutputElfType(const CopyConfig &Config,
+                                          const Reader &Reader) {
----------------
alexshap wrote:
> rupprecht wrote:
> > alexshap wrote:
> > > it looks like now or even in the future None  here is not "recoverable" anyway
> > > (down the line we exit with an error),
> > > maybe it would be better to return the enum directly instead ?
> > > (and if the output format is "binary" it will error("Unsupported output format ...")).
> > The unrecoverable error that's thrown when this optional doesn't have a value comes immediately after a check that Config.OutputFormat == "binary". So returning None is valid in that case.
> yes, that's exactly what i was saying -  but the point was different - wouldn't returning enum directly make the code a bit simpler ?
I think it would be OK to return an error/return Expected<ElfType>/etc. if this were called from a scope where we were guaranteed that the output is an elf binary. But, it's not. I think if the output format is "binary", the logical thing for an API to do when asked "What's your elf type?" in a generic context would be return <None>, because the output isn't an ELF binary at all, right?

i.e., the relevant code distilled is:
```
static void ExecuteElfObjcopyOnBinary() {
  ...
  Optional<ElfType> OutputElfType = GetOutputElfType();
  std::unique_ptr<Writer> Writer = CreateWriter(..., Config, OutputElfType);
  ...
}

static std::unique_ptr<Writer>CreateWriter(..., Config, Optional<OutputElfType>) {
  if (Config.OutputFormat == "binary") // Return binary writer, and OutputElfType is never even looked at
  else // ... Use OutputElfType to make an ELFWriter ...
}
```

Having GetOutputElfType() return an error if the output is binary will break that case. Having it return Expected<ElfType> would work, although it would be weird -- you'd have to only check it in CreateWriter only after you've checked that the output format isn't binary, so I think Optional<ElfType> is cleaner.

Given the state the code is in today, it's always possible to derive an ELF type, but only because --input-target is currently ignored, and the inputs are implicitly assumed to be ELF objects from which we can derive a type. I think the code is simpler when optimized for reading, and returning an Optional<ElfType> makes it clear that the output might not be ELF.

Sorry for the long reply -- I'm OK with making some changes here, but I'm not sure which direction you want this to go in.


Repository:
  rL LLVM

https://reviews.llvm.org/D50117





More information about the llvm-commits mailing list