[PATCH] D64668: [llvm-lipo] Implement -info

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added inline comments.


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:270
 
-LLVM_ATTRIBUTE_NORETURN
-static void printArchs(ArrayRef<OwningBinary<Binary>> InputBinaries) {
+static void printBinaryArchs(const Binary *Binary, raw_ostream &OutputStream) {
   // Prints trailing space for compatibility with cctools lipo.
----------------
`OS` is the naming convention for the output stream


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:286
 
-  outs() << "\n";
+  OutputStream << "\n";
+}
----------------
I think it may be nicer to change L280-L286 to something like:

```
    OS << '\n';
    return;
  }

  auto *O = cast<MachOObjectFile>(Binary);
  OS << getArchString(*O) << " \n";
}
```


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:318
+      llvm_unreachable("Unexpected binary format");
+    }
+  }
----------------
Similar to above, you can use `continue` in the universal case and unindent the else case (make the `Binary->isMachO()` an assertion,  and drop the unreachable.


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:547
+    printInfo(InputBinaries);
+    break;
   case LipoAction::ThinArch:
----------------
Is there validation being applied here?  I think that you should add a test case for invalid input as well (that is, pass ELF files to `lipo` and ensure that we handle that gracefully (right now, it feels like we may just abort?).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64668