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

[Anusha Basana via Phabricator via llvm-commits]

anushabasana added a comment.

Ping



================
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");
----------------
alexshap wrote:
> alexshap wrote:
> > smeenai wrote:
> > > 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?
> > not sure it really matters, the thing is that in general lipo can have multiple input files,
> > different actions expect different number of input files, thus since in main() we dispatch handling of particular commands to different subroutines  i think it's clear / consistent enough. I'd prefer to not to make that "top-level" code less readable, although it's simple either way so i don't insist.
> discussed with @compnerd and we agreed on this, might be worth adding a comment in the code (in main) that the details/specifics of handling particular options (i.e. the number of expected input files) are inside the corresponding subroutines.
>  
what sort of comment did you have in mind? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62753