[PATCH] D88400: [llvm-objcopy][MachO] Add support for universal binaries

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Just a few minor comments from me, but I don't really follow the logic as always, so this will need others to comment on it.



================
Comment at: llvm/test/tools/llvm-objcopy/MachO/universal-object.test:31
+## Case 4: try to copy a universal object file contaning a bitcode slice.
+# RUN: echo 'target triple = "arm64-apple-ios8.0.0"' >%t.arm64.ll
+# RUN: llvm-as %t.arm64.ll -o %t.bitcode
----------------
You could also just inline this text directly in the file and feed `%s` as the input to `llvm-as`.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:423
+    }
+    consumeError(ArOrErr.takeError());
+
----------------
Here and the other `consumeError` site probably deserve comments explaining why it's okay to throw away the error reported.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOObjcopy.cpp:429
+      return createStringError(std::errc::invalid_argument,
+                               "Universal Mach-O object '%s' contains a slice "
+                               "which is not a Mach-O object or an archive",
----------------
Preferred error message style starts with a lower-case letter.

Is there anything that allows us to uniquely identify which slice is the problematic one (e.g. a member name or similar)? If so, it might be helpful to report it as part of the message.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88400