[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:
> > 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 ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88113