[PATCH] D61927: [tools]Introduce llvm-lipo

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added a comment.

Looks good, though I'm not super familiar with the libObject APIs (yet) :)



================
Comment at: test/tools/llvm-lipo/verify-arch-macho-binary.test:3
+
+# RUN: llvm-lipo %t -verify_arch i386
+# RUN: llvm-lipo %t --verify_arch i386
----------------
lipo only supports this form of specifying flags (single dash, no equals). Do we want to support the others?


================
Comment at: test/tools/llvm-lipo/verify-arch-macho-binary.test:22
+--- !mach-o
+FileHeader:      
+  magic:           0xFEEDFACE
----------------
Can we strip this down as much as we can? E.g. https://reviews.llvm.org/diffusion/L/browse/llvm/trunk/test/tools/llvm-objdump/AArch64/Inputs/arm64e.macho.yaml


================
Comment at: test/tools/llvm-lipo/verify-arch-universal-binary.test:10
+
+--- !fat-mach-o
+FatHeader:       
----------------
Same comment about minimizing this as much as possible.


================
Comment at: tools/llvm-lipo/llvm-lipo.cpp:33
+static cl::list<std::string> VerifyArch(
+    "verify_arch", cl::ZeroOrMore,
+    cl::desc(
----------------
Shouldn't this be OneOrMore?


================
Comment at: tools/llvm-lipo/llvm-lipo.cpp:74
+static void
+verifyArch(const SmallVector<OwningBinary<Binary>, 1> &InputBinaries) {
+  assert(!InputBinaries.empty() &&
----------------
Would it be better to use a `SmallVectorImpl` as the parameter type?


================
Comment at: tools/llvm-lipo/llvm-lipo.cpp:81
+  for (StringRef Arch : VerifyArch)
+    if (Triple::ArchType::UnknownArch == Triple(Arch).getArch())
+      reportError(Twine("Invalid architecture: ") + Arch);
----------------
Super nit: I think this would read more naturally if the left and right hand sides of the `==` were switched.


================
Comment at: tools/llvm-lipo/llvm-lipo.cpp:82
+    if (Triple::ArchType::UnknownArch == Triple(Arch).getArch())
+      reportError(Twine("Invalid architecture: ") + Arch);
+
----------------
Do we need the explicit `Twine` constructor here? I thought there was an `operator+` for `const char *` and `StringRef`.


================
Comment at: tools/llvm-lipo/llvm-lipo.cpp:95
+    for (StringRef Arch : VerifyArch)
+      if (O->getArch() != Triple(Arch).getArch())
+        reportError(Twine(O->getFileName()) + " does not contain " + Arch);
----------------
`Triple(Arch).getArch()` is used a bunch. Should we save it to a variable?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61927