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

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: llvm/tools/llvm-lipo/llvm-lipo.cpp:301
+static void printInfo(ArrayRef<OwningBinary<Binary>> InputBinaries) {
+  std::string UniversalOutputBuffer;
+  raw_string_ostream UniversalOutputStream(UniversalOutputBuffer);
----------------
compnerd wrote:
> alexshap wrote:
> > compnerd wrote:
> > > anushabasana wrote:
> > > > alexshap wrote:
> > > > > alexshap wrote:
> > > > > > khm, the same effect (grouping universal/regular files) could be achieved a bit simpler, what would you say to the following (no need for std::string, raw_string_ostream, ThinOutputBuffer, ThinOutputStream, UniversalOutputBuffer, UniversalOutputStream, etc)
> > > > > >      
> > > > > >     for (const auto &IB : InputBinaries)
> > > > > >         if (IB.getBinary()->isMachOUniversalBinary()) {
> > > > > >              outs() << "Architectures in the fat file: " << Binary->getFileName() 
> > > > > >                        << " are: ";
> > > > > >              printBinaryArchs(IB.getBinary(), outs());
> > > > > >         }
> > > > > >      for (const auto &IB : InputBinaries)
> > > > > >         if (IB.getBinary()->isMachO) {
> > > > > >              outs() << "Non-fat file: " << IB.getBinary()->getFileName()
> > > > > >                        << " is architecture: ";
> > > > > >              printBinaryArchs(IB.getBinary(), outs());
> > > > > >         }            
> > > > > cc: @mtrent 
> > > > > 1. The wording (especially in the second case) appears to be very weird, maybe it's worth to change it and make it more human-readable ? (I suspect `-info` is not really widely used in scripts, so probably this incompatibility might not be a big issue).
> > > > > 2. Maybe we don't even need to group anything here (for the same reasons)
> > > > I could remove the string and OS for Universal files and just pass in outs() to cut down there.
> > > > I originally had 2 for loops but I thought the multiple OS would be better so we would only have to do one pass over the input binaries.
> > > > I'm not sure what is more legible, I don't have a strong preference.
> > > @alexshap - I think that I would rather that we go with stronger claims:
> > > 
> > > ```
> > > for (const auto &IB : InputBinaries) {
> > >   Binary *B = IB.getBinary();
> > >   assert(B->isMachO() && "expected MachO binary");
> > >   outs() << "Non-fat file: " << B->getFileName() << " is architecture: ";
> > >   printBinaryArchs(B, outs());
> > > }
> > > ```
> > I don't think that performance is really a concern here (and in any case IO + allocations are more expensive than two "for" loops are), so I'd prefer to make this code as simple as possible, 
> > in particular, get rid of these intermediate strings  and buffers (I'm referring to ThinOutputBuffer, ThinOS, etc)
> > 
> It seemed that this was confusing for @alexshap, what I meant is that this could be written as:
> 
> ```
>   for (const auto &IB : InputBinaries) {
>     if (IB.getBinary()->isMachOUniversalBinary()) {
>       ...
>     }
>   }
>   for (const auto &IB : InputBinaries) {
>     if (!IB.getBinary()->isMachOUniversalBinary()) {
>       ...
>     }
>   }
> ```
> 
> which makes it more explicit that the two loops explicitly go over everything just filtering on a specific condition.
I've definitely seen `-info` be used in some scripts, particularly since `-archs` is a more recent invention, I think. I think it's best if we match lipo output here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64668