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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:291
 
+static Optional<ElfType> GetOutputElfType(const CopyConfig &Config,
+                                          const Reader &Reader) {
----------------
jhenderson wrote:
> rupprecht wrote:
> > 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.
> If it's unrecoverable, should we actually return an Expected?
though this is probably not a big deal, i think Optional is not the right choice here because of the reasons mentioned above. I think it would be better to either return enum directly or make use of Expected<...>.


Repository:
  rL LLVM

https://reviews.llvm.org/D50117