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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/CopyConfig.h:239-242
+      if (Expected<elf::ELFCopyConfig> ELFConfig = elf::parseConfig(*this))
+        ELF = *ELFConfig;
+      else
         return ELFConfig.takeError();
----------------
jhenderson wrote:
> Why did this code change? It looks semantically identical, and I think it's actually a little less readable.
I personally found this way more readable. I used it elsewhere and changed in this place so that it looks equal to other places. Will change back.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:358
   MachOReader Reader(In);
-  std::unique_ptr<Object> O = Reader.create();
-  if (!O)
-    return createFileError(
-        Config.InputFilename,
-        createStringError(object_error::parse_failed,
-                          "unable to deserialize MachO object"));
-
-  if (Error E = handleArgs(Config, *O))
-    return createFileError(Config.InputFilename, std::move(E));
-
-  // Page size used for alignment of segment sizes in Mach-O executables and
-  // dynamic libraries.
-  uint64_t PageSize;
-  switch (In.getArch()) {
-  case Triple::ArchType::arm:
-  case Triple::ArchType::aarch64:
-  case Triple::ArchType::aarch64_32:
-    PageSize = 16384;
-    break;
-  default:
-    PageSize = 4096;
-  }
+  if (Expected<std::unique_ptr<Object>> O = Reader.create()) {
+    if (!*O)
----------------
jhenderson wrote:
> Cleaner code would be to just do:
> ```
> Expected<std::unique_ptr<Object>> O = Reader.create();
> if (!ObjOrErr)
>   return O.takeError();
> ...
> ```
Ok, will use that pattern.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:360-363
+      return createFileError(
+          Config.InputFilename,
+          createStringError(object_error::parse_failed,
+                            "unable to deserialize MachO object"));
----------------
jhenderson wrote:
> If I'm following things correctly, this piece of code is dead? Even if there were errors in the `create` method, they should now be reported via the `Expected` being returned. If it is dead, please remove it. If not, I think we should change the behaviour to report the error in the same manner as the other errors.
yes, it looks dead. I left it just in case. Will remove.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.h:27-42
+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 + "': ";
----------------
jhenderson wrote:
> If I'm not mistaken, this whole function can just be replaced with a single `createStringError()` call, similar to what we often do in llvm-readobj (see e.g. https://github.com/llvm/llvm-project/blob/master/llvm/tools/llvm-readobj/ELFDumper.cpp#L5348).
OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88113