[PATCH] D63341: [llvm-lipo] Implement -thin

[Michael Trent via Phabricator via llvm-commits]

mtrent added a comment.

I'll have a look Monday / early next week. Meanwhile, here are some quick thoughts.



================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:165
+    if (C.OutputFile.empty())
+      reportError("thin expects a single output file to be specified");
+    C.ThinArchType = ActionArgs[0]->getValue();
----------------
nit. s/to be specified//


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:178
   SmallVector<OwningBinary<Binary>, 1> InputBinaries;
+  // TODO: Add compatibility for archive files
   for (StringRef InputFile : InputFiles) {
----------------
not really sure what the right protocol for this is in LLVM-land. Should you file a bug in some LLVM-specific bug tracker? Or maybe should you print a warning/error saying a format is not (yet) supported? 

I thought Archive files were SymbolicFiles and so they "should" be supported, but maybe you're using the wrong level of API? (I could be wrong ... just asking.)


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:273
+  if (isa<MachOObjectFile>(InputBinaries.front().getBinary())) {
+    reportError("Input file must be a universal binary when the -thin flag is "
+                "specified");
----------------
Nit / optional. Since llmv-lipo is meant to be a drop-in replacement for lipo, this should ideally produce "input file (hello) must be a fat file when the -thin option is specified"



================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:275
+                "specified");
+    exit(EXIT_SUCCESS);
+  }
----------------
Since llmv-lipo is meant to be a drop-in replacement for lipo, this must be EXIT_FAILURE


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:283
+    reportError(
+        "Input file does not contain the specified thin architecture : " +
+        ThinArchType);
----------------
Nit / optional. Since llmv-lipo is meant to be a drop-in replacement for lipo, this should ideally produce: "fat input file (<FILE NAME>) does not contain the specified architecture (<ARCH NAME>) to thin it to"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63341