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

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: llvm/test/tools/llvm-lipo/archs-macho-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 | FileCheck %s
----------------
This should be worded better.


================
Comment at: llvm/test/tools/llvm-lipo/archs-macho-binary-unknown.test:6
+# CHECK: unknown(151,3)
+--- !mach-o
+FileHeader:
----------------
Nit: add a newline above this.


================
Comment at: llvm/test/tools/llvm-lipo/archs-macho-binary.test:10-11
+
+# RUN: not llvm-lipo %t -archs -verify_arch i386 2>&1 | FileCheck --check-prefix=MULTIPLE_FLAGS %s
+# MULTIPLE_FLAGS: only one of the following actions can be specified: -archs -verify_arch
+
----------------
This one isn't specific to `-archs`; it should happen when combining any two flags.

There's an existing `help-message.test`. Let's rename that to something like `command-line.test` and move this check to there.


================
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
----------------
compnerd wrote:
> Nit: double space after "as"
Comment should be worded better.


================
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;
----------------
compnerd wrote:
> This is confusing.  Where is this used?
Line 64 below references `LIPO_##PREFIX`, and for an option group the prefix is `nullptr`. You should add a comment.


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:135
+
   if (InputArgs.hasArg(LIPO_verify_arch)) {
     for (auto A : InputArgs.getAllArgValues(LIPO_verify_arch))
----------------
At this point, we should have a single arg inside `ActionArgs`, so we can just dispatch based on that instead of rechecking InputArgs. Something like

```
switch (ActionArgs[0]->getOption().getID()) {
case LIPO_verify_arch:
  ...
  break;
case LIPO_archs:
  ...
  break;
default:
  llvm_unreachable("unexpected action argument");
}
```


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:154
+
+  llvm_unreachable("llvm-lipo action unspecified");
 }
----------------
compnerd wrote:
> 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?
Lines 123 through 133 should ensure we have a single action argument of the expected type. My switch statement suggestion above should make that structure clearer.


================
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");
----------------
compnerd wrote:
> Why not just make the parameter an `OwningBinary<Binary>` rather than the `ArrayRef` and then assert that a single entry is provided?
Good idea. We'd probably also wanna take the parameter by const reference?


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:227
+  else
+    llvm_unreachable("Unexpected binary format");
+
----------------
compnerd wrote:
> 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.
`readInputBinaries` above should have ensured we have a binary in the correct format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62753