[PATCH] D88113: [llvm-objcopy][NFC] refactor error handling. part 1.

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOReader.cpp:89
     if (!SecRef)
-      reportError(MachOObj.getFileName(), SecRef.takeError());
+      return createStringError(errc::invalid_argument,
+                               "'" + MachOObj.getFileName() +
----------------
avl wrote:
> alexshap wrote:
> > avl wrote:
> > > alexshap wrote:
> > > > jhenderson wrote:
> > > > > avl wrote:
> > > > > > jhenderson wrote:
> > > > > > > Can you use the error code associated with `SecRef` instead of `errc::invalid_argument` here?
> > > > > > As far as I understand that could be done in that way:
> > > > > > 
> > > > > > 
> > > > > > ```
> > > > > > inline Error errorWithContext(StringRef Context, Error E) {
> > > > > >   assert(E);
> > > > > >   std::string Buf;
> > > > > >   raw_string_ostream OS(Buf);
> > > > > >   Optional<std::error_code> EC;
> > > > > >   handleAllErrors(std::move(E), [&](const ErrorInfoBase &EI) {
> > > > > >     OS << "'" + Context + "': ";
> > > > > >     if (!EC)
> > > > > >       EC = EI.convertToErrorCode();
> > > > > >     EI.log(OS);
> > > > > >     OS << "\n";
> > > > > >   });
> > > > > >   OS.flush();
> > > > > > 
> > > > > >   return createStringError(*EC, Buf);
> > > > > > }
> > > > > > ```
> > > > > > 
> > > > > > but you`ve asked me to not create a special routine for that.
> > > > > I misremembered how the interface worked. There's an `errorToErrorCode()` function, but that consumes the input `Error`, which we don't want. Leave it as-is.
> > > > I'm wondering - why not simply bubble up the error ?
> > > My guess is that original intention is to add file name MachOObj.getFileName to have better reporting. If we would like to add filename then we could not bubble up original error.
> > On the line 359 (in MachOObjcopy.cpp) we can use createFileError (similarly to how we deal with handleArgs) and this would probably enable us to get rid of these conversions / the code would simplify a bit. 
> > P.S. since we were not propagating errors before this change we had to adjust the message in multiple places
> > what would you say to this ?
> Ah, you mean to use createFileError instead of createString Error. Right, that would be better usage. 
> 
> As to adjusting the message in multiple places - did you mean checking other usages of createStringError and replace them with createFileError if applicable ? It looks like all other usages of createStringError inside MachO subfolder are not suitable to replacing with createFileError.
I was referring to the lines 359/360 in MachOObjcopy.cpp

So here you'd simply return Data.takeError() etc, 
and in MachOObjcopy.cpp we would have

```
  Expected<std::unique_ptr<Object>> O = Reader.create();
  if (!O)
    return createFileError(Config.InputFilename, O.takeError());

  if (Error E = handleArgs(Config, **O))
    return createFileError(Config.InputFilename, std::move(E));
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88113