[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);
----------------
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)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64668