[PATCH] D62753: [llvm-lipo] Implement -archs

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added inline comments.


================
Comment at: llvm/test/tools/llvm-lipo/archs-macho-binary-unknown.test:1
+# RUN: yaml2obj %s > %t
+
----------------
Can you avoid the temporary using:

`yaml2obj %s | llvm-lip - -archs | FileCheck %s`


================
Comment at: llvm/test/tools/llvm-lipo/archs-universal-binary-unknown.test:1
+# RUN: yaml2obj %s > %t
+
----------------
Similar


================
Comment at: llvm/test/tools/llvm-lipo/archs-universal-binary-unknown.test:3
+
+# Tests that the output for an unknown architecture is the same as  to check cctools lipo
+# RUN: llvm-lipo %t -archs 2>&1 | FileCheck %s
----------------
Nit: double space after "as"


================
Comment at: llvm/test/tools/llvm-lipo/archs-universal-binary.test:1
+# RUN: yaml2obj %s > %t
+
----------------
Similar


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:56
 
+const char *const *LIPO_nullptr = nullptr;
 #define PREFIX(NAME, VALUE) const char *const LIPO_##NAME[] = VALUE;
----------------
This is confusing.  Where is this used?


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:154
+
+  llvm_unreachable("llvm-lipo action unspecified");
 }
----------------
I don't think that this should be unreachable - if the action is unspecified, that can occur normally in the way that the user invokes the tool.  Perhaps this should be a proper error?


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:216
+LLVM_ATTRIBUTE_NORETURN
+static void printArchs(ArrayRef<OwningBinary<Binary>> InputBinaries) {
+  assert(InputBinaries.size() == 1 && "Incorrect number of input binaries");
----------------
Why not just make the parameter an `OwningBinary<Binary>` rather than the `ArrayRef` and then assert that a single entry is provided?


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:227
+  else
+    llvm_unreachable("Unexpected binary format");
+
----------------
I don't think that this is necessarily unreachable - an invalid binary being given to the tool could hit this.  This should be a proper error.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62753