[PATCH] D87987: [llvm-objcopy][NFC] refactor error handling. part 3.

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

I'm **really** happy to see this. I tried removing the `error()` methods from `llvm-objcopy.h` a year or two ago and didn't have the patience to work through all the plumbing seen here.

This approach LGTM to me. I just have a couple nits.



================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:561-569
+Expected<CompressedSection>
+CompressedSection::create(ArrayRef<uint8_t> CompressedData,
+                          uint64_t DecompressedSize,
+                          uint64_t DecompressedAlign) {
+  CompressedSection Section(CompressedData, DecompressedSize,
+                            DecompressedAlign);
+
----------------
Nothing's happening to this variable, so I think `return CompressedSection(...)` looks better here


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:578-585
+  if (Error Err = zlib::compress(
           StringRef(reinterpret_cast<const char *>(OriginalData.data()),
                     OriginalData.size()),
           CompressedData))
-    reportError(Name, std::move(E));
+    OutErr = createStringError(llvm::errc::invalid_argument,
+                               "'" + Name + "': " + toString(std::move(Err)));
 
----------------
Once we detect an error, we probably don't want to go on and process the rest of this function, e.g. use of `CompressedData` may not be valid.

You can't return early from the constructor IIUC, but maybe something like:

```
if (Error Err = zlib::compress(...)) {
  OutErr = ...
} { else {
  // Rest of constructor
}
```


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1750
+
+  llvm_unreachable("unknown section sh_type");
 }
----------------
Is this actually necessary? I think the above switch statement handles all cases (including a default case), and if any of the types break instead of returning, you should get a `-Wreturn` error trying to build this.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.cpp:1905-1908
+  if (Error E = readProgramHeaders(*HeadersFile))
+    return E;
+
+  return Error::success();
----------------
nit: this pattern can be reduced:
```
if (Error E = foo()) return E;
return Error::success();
```
->
```
return foo();
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87987