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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap 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);
----------------
smeenai wrote:
> 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.
ok, then let's leave it as is


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64668