[PATCH] D56930: [llvm-objcopy] Return Error from Buffer::allocate(), [ELF]Writer::finalize(), and [ELF]Writer::commit()

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht marked an inline comment as done.
rupprecht added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/Buffer.cpp:25
+  if (!BufferOrErr)
+    return createFileError(getName(), BufferOrErr.takeError());
   Buf = std::move(*BufferOrErr);
----------------
alexshap wrote:
> just in case: why do we use createFileError here instead of propagating the original error BufferOrErr.takeError() ?
Left a comment inline explaining why.

On the base side, this was manually calling `error(FileName + ": " + Message)` to include the filename in the error, which is analagous to `reportError(FileName, Error)` used elsewhere in llvm-objcopy. If we want to bubble up Errors to the top level instead of calling error/reportError with the filename here, we need to wrap it in FileError.
I guess there might be reasons why FileOutputBuffer methods don't return FileErrors. It would be nice if this code didn't assume that FileError *isn't* being used, e.g.

```
// Wraps errors into FileErrors, but doesn't re-wrap FileErrors
Error maybeWrapFileError(StringRef FileName, Error Err) {
  return Err.isA<FileError>() ? std::move(Err) : createFileError(FileName, std::move(Err));
}
```
Depending on how common this is when migrating the rest of these, I might put this utility somewhere in libSupport. For now, it's just these two spots.


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

https://reviews.llvm.org/D56930